fix: resolve next-server from next app directory and not from plugin #2059
Conversation
✅ Deploy Preview for netlify-plugin-nextjs-export-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-nx-monorepo-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-static-root-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-i18next-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-plugin-edge-middleware ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for nextjs-plugin-custom-routes-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-plugin-canary ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-next-auth-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
| const nextServerModuleRelativeLocation = nextServerModuleAbsoluteLocation | ||
| ? relative(functionDir, nextServerModuleAbsoluteLocation) | ||
| : undefined | ||
|
|
There was a problem hiding this comment.
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)
| // This is a string, but if you have the right editor plugin it should format as js | ||
| javascript/* javascript */ ` | ||
| if (!${JSON.stringify(nextServerModuleRelativeLocation)}) { | ||
| throw new Error('Could not find Next.js server') |
There was a problem hiding this comment.
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.
Yeah, i think that makes sense tbh. We only need the server at runtime so let's go with this.
|
EDIT: this is now done |
| // eslint-disable-next-line max-params | ||
| const makeHandler = (conf: NextConfig, app, pageRoot, page, NextServer: NextServerType) => { |
There was a problem hiding this comment.
Since another parameter is being added, consider converting the parameters to one parameter object.
| // eslint-disable-next-line max-params | |
| const makeHandler = (conf: NextConfig, app, pageRoot, page, NextServer: NextServerType) => { | |
| const makeHandler = ({ conf, app, pageRoot, page, NextServer }: MakeHandlerParams) => { |
and update at the call sites.
| // eslint-disable-next-line import/no-dynamic-require | ||
| extractConstValue = require(findModuleFromBase({ | ||
| paths: [appDir], | ||
| candidates: ['next/dist/build/analysis/extract-const-value'], | ||
| }) ?? 'next/dist/build/analysis/extract-const-value') |
There was a problem hiding this comment.
This is a bit messy - especially part after ?? - it's done because findModuleFromBase can return null, and calling require(null) won't result in MODULE_NOT_FOUND error that we handle below - so the reason for ?? part is to actually produce above error in case module can't be resolved
There was a problem hiding this comment.
[sand] you could potentially move the try/catch from findModuleFromBase to getServerFile (which is the only place that findModuleFromBase is used) and then you don't have to the juggling with ??
There was a problem hiding this comment.
I decided to do 04f3164 instead - I looked at how throwing would look like in findModuleFromBase and realized that removing try/catch there would result in throwing in first missed candidate, and to make it somewhat work it would be quite messy with storing exceptions and rethrowing them when we completed iterating over candidates and still didn't find one.
Also thinking more - this would change semantics of findModuleFromBase which is not end of the world, but I prefer to avoid changing shared utils behavior if it can be addressed in single place that would benefit from it (and possibly cause problems in all the other places)
| // I have no idea what eslint is up to here but it gives an error | ||
| // eslint-disable-next-line no-shadow | ||
| export const enum ApiRouteType { | ||
| SCHEDULED = 'experimental-scheduled', | ||
| BACKGROUND = 'experimental-background', | ||
| } |
There was a problem hiding this comment.
This (including comment) was moved from another module due to cyclic imports problem
There was a problem hiding this comment.
I pulled this locally and same thing. The error is weird because it says it's shadowing itself. 🙃
There was a problem hiding this comment.
// I have no idea what eslint is up to here but it gives an error
// eslint-disable-next-line no-shadow
This is needed to be moved as well and not just type - same error happen in current location when you remove comments
There was a problem hiding this comment.
typescript-eslint/typescript-eslint#2483 is the reason - enum stuff is problematic with non typescript no-shadow rule - I can fix this either here or separately
| @@ -0,0 +1,29 @@ | |||
| name: Next Runtime Integration Tests | |||
There was a problem hiding this comment.
This is not really integration test - it's really more e2e, but "e2e" is somewhat taken by tests we have copied from Next - so I went with integration for now, but maybe we just need to agree on how to refer to e2e tests that are not copied from next and I would love to adjust naming in this PR to reflect that
There was a problem hiding this comment.
Would it make sense to rename the Next tests 'upstream' tests?
There was a problem hiding this comment.
Yeah, I think it make sense, but at this point I'd rather ship those as-is and do renaming in separate standalone PR to not keep the fix hostage
| @@ -0,0 +1,29 @@ | |||
| name: Next Runtime Integration Tests | |||
There was a problem hiding this comment.
Would it make sense to rename the Next tests 'upstream' tests?
| } catch { | ||
| // Ignore the error | ||
| } | ||
| } |
There was a problem hiding this comment.
[sand] instead of an additional for loop, consider pushing the current directory '.' to the paths array
There was a problem hiding this comment.
I tried, and it's actually not that simple. Using just ., __dirname etc in paths alone doesn't replicate behavior of require.resolve (when using default paths). We can get what default would use by calling require.resolve.paths(candidate) and we can push all that to our custom paths sure, but I never actually used that function before in production and I'm not sure of its sharp edges so would prefer to avoid going that route.
Just for some more details on that - here's example what default paths look like generally - it first starts with directory of current module + /node_modules and then go up directory tree, later seems to be checking some globals (??) etc:
> pwd
/Users/misiek/dev/next-runtime
> node -e "console.log(require.resolve.paths('next'))"
[
'/Users/misiek/dev/next-runtime/node_modules',
'/Users/misiek/dev/node_modules',
'/Users/misiek/node_modules',
'/Users/node_modules',
'/node_modules',
'/Users/misiek/.node_modules',
'/Users/misiek/.node_libraries',
'/Users/misiek/.nvm/versions/node/v18.14.0/lib/node'
]
So TL/DR it's not as simple as . and it might potentially introduce unknown problems that I would prefer to avoid
There was a problem hiding this comment.
Thanks for the explanation @pieh, got it 👍🏼
| // eslint-disable-next-line import/no-dynamic-require | ||
| extractConstValue = require(findModuleFromBase({ | ||
| paths: [appDir], | ||
| candidates: ['next/dist/build/analysis/extract-const-value'], | ||
| }) ?? 'next/dist/build/analysis/extract-const-value') |
There was a problem hiding this comment.
[sand] you could potentially move the try/catch from findModuleFromBase to getServerFile (which is the only place that findModuleFromBase is used) and then you don't have to the juggling with ??
| // This is a string, but if you have the right editor plugin it should format as js | ||
| javascript/* javascript */ ` | ||
| if (!${JSON.stringify(nextServerModuleRelativeLocation)}) { | ||
| throw new Error('Could not find Next.js server') |
There was a problem hiding this comment.
Yeah, i think that makes sense tbh. We only need the server at runtime so let's go with this.
…y-when-resolving-server-for-handler
| // I have no idea what eslint is up to here but it gives an error | ||
| // eslint-disable-next-line no-shadow | ||
| export const enum ApiRouteType { | ||
| SCHEDULED = 'experimental-scheduled', | ||
| BACKGROUND = 'experimental-background', | ||
| } |
There was a problem hiding this comment.
I pulled this locally and same thing. The error is weird because it says it's shadowing itself. 🙃
|
|
||
| module.exports = { | ||
| onPostBuild: async () => { | ||
| const movedDir = path.join(process.cwd(), `next-app`, `node_modules2`) |
There was a problem hiding this comment.
I'm assuming this is coming from the tests pulled in, but where does node_modules2 get set? This appears to be the only place it's mentioned. 🤔
There was a problem hiding this comment.
Ah sorry - this is not pulled in test - it's just new one that uses netlify serve and this "build plugin" is meant to get rid of node_modules after functions were bundled but before they started to be served. However when I just deleted node_modules it was problematic, sometimes getting ENOENT errors so instead of deleting it I just move node_modules to node_modules2 so it no longer is used. The reason to check if node_modules2 exist and deleting it if it does is because running the same test twice in a row would crash otherwise.
And the reason to remove node_modules are just functions bundling quirks I encountered:
I did use absolute path to next-server (so something like /Users/misiek/dev/repo/next-app/node_modules/next/<path-to-next-server>) when generating lambda handler at some point and it worked locally, but not when deployed, because functions bundler doesn't seem to handle absolute paths correctly and left those imports as they are instead of bundling it. Things appeared to be working locally because those node_modules still existed.
Co-authored-by: Nick Taylor <[email protected]>
LekoArts
deleted the
fix/use-next-directory-when-resolving-server-for-handler
branch
Summary
This PR adds functionality to resolve
next/dist/server/next-serverfrom the Next app directory (not to be confused withappDirinside pages), and not from the location of thenext-runtime.Test plan
A new integration test was added.
Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal
Fixes #2024
Standard checks:
🧪 Once merged, make sure to update the version if needed and that it was published correctly.