Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix: resolve next-server from next app directory and not from plugin #2059
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: resolve next-server from next app directory and not from plugin #2059
Changes from 9 commits
8d8d8ae
e7ae048
15c1bdd
4b64c17
1933824
e06157d
ffae6a2
d50c047
c5a4afd
48468bc
33c28ae
be8bc6e
caef767
56aa4f7
1e48952
22e2fa2
6e229c9
74ab70e
e4fadf4
1774f47
bbff3a9
ade1a2e
610d73f
917d3ce
1b01485
04f3164
92dcf3a
66a9670
6e4d78a
8f71783
9efa6cb
a92dbac
767dca9
40c6469
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if
nextServerModuleAbsoluteLocation
is falsy we could warn here that non-SSG routes will fail (tho that might be noisy, so if we want warning like that maybe we add one once we know if we need lambdas for routes)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since another parameter is being added, consider converting the parameters to one parameter object.
and update at the call sites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 610d73f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In current state of this PR, this is thrown on lambda invocation and not during the build, so it restores the that behavior prior to #1950 on when the error is thrown - not the hill I will die on and I can move it back to build time.
The other changes in this PR make it more likely to find
next-server
than before, but I don't have 100% confidence in it, so that's why I move this error throwing back here as that was behavior for a long time.Without that move, builds for fully static sites that hit current error are just blocked until we implement logic to determine wether we need page route handlers at all and skip it altogether if not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, i think that makes sense tbh. We only need the server at runtime so let's go with this.