fix: don't ever execute middleware in lambda #2249
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for netlify-plugin-nextjs-nx-monorepo-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for netlify-plugin-nextjs-static-root-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for next-plugin-edge-middleware ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
github-actions
bot
added
the
type: chore
❌ Deploy Preview for netlify-plugin-nextjs-next-auth-demo failed.
|
✅ Deploy Preview for netlify-plugin-nextjs-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for nextjs-plugin-custom-routes-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for next-i18next-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for next-plugin-canary ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for netlify-plugin-nextjs-export-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
pieh
force-pushed
the
middleware-preview-test
branch
4 times, most recently
from
9fdd493 to
7e71458
Compare
pieh
force-pushed
the
middleware-preview-test
branch
from
7e71458 to
a5f2f74
Compare
pieh
commented
Aug 10, 2023
Comment on lines
+1
to
+34
| describe('Preview Mode', () => { | ||
| it('enters and exits preview mode', () => { | ||
| Cypress.Cookies.debug(true) | ||
| cy.getCookies().then((cookie) => cy.log('cookies', cookie)) | ||
|
|
||
| cy.intercept('/previewTest', (req) => { | ||
| req.continue((res) => { | ||
| expect(res.headers?.['x-middleware-executed']).to.equal('true') | ||
| }) | ||
| }).as('previewTestVisit') | ||
|
|
||
| // preview mode is off by default | ||
| cy.visit('/previewTest') | ||
| cy.findByText('Is preview? No', { selector: 'h1' }) | ||
|
|
||
| // enter preview mode | ||
| cy.request('/api/enterPreview').then((response) => { | ||
| expect(response.body).to.have.property('name', 'preview mode') | ||
| }) | ||
|
|
||
| // exptected content is rendered | ||
| cy.visit('/previewTest') | ||
| cy.findByText('Is preview? Yes!', { selector: 'h1' }) | ||
|
|
||
| // exit preview mode | ||
| cy.request('/api/exitPreview') | ||
| cy.visit('/previewTest') | ||
| cy.findByText('Is preview? No', { selector: 'h1' }) | ||
|
|
||
| // we should hit /previewTest 3 times (before entering preview, after entering preview, after exiting preview) | ||
| // this assertion is mainly to ensure interception works and assertion on response header is made | ||
| cy.get('@previewTestVisit.all').should('have.length', 3) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
This test does assert things correctly, but the version of Next used in middleware demo actually doesn't have the bug included, so the fix is only relevant to older next versions which we don't run test against currently :/
I think it's still worthwhile to include the test, but we should figure out test setup to run against more versions of next.js.
[email protected] does fail this test without the fix ( https://cloud.cypress.io/projects/yn8qwi/runs/2299/overview?roarHideRunsWithDiffGroupsAndTags=1 ) but with setup as-is we won't be using that version of next :/
There was a problem hiding this comment.
It's good to have this like you mentioned and if we get around to running these tests on older versions, this will definitely be helpful.
pieh
changed the title
nickytonline
approved these changes
Aug 10, 2023
Comment on lines
+1
to
+34
| describe('Preview Mode', () => { | ||
| it('enters and exits preview mode', () => { | ||
| Cypress.Cookies.debug(true) | ||
| cy.getCookies().then((cookie) => cy.log('cookies', cookie)) | ||
|
|
||
| cy.intercept('/previewTest', (req) => { | ||
| req.continue((res) => { | ||
| expect(res.headers?.['x-middleware-executed']).to.equal('true') | ||
| }) | ||
| }).as('previewTestVisit') | ||
|
|
||
| // preview mode is off by default | ||
| cy.visit('/previewTest') | ||
| cy.findByText('Is preview? No', { selector: 'h1' }) | ||
|
|
||
| // enter preview mode | ||
| cy.request('/api/enterPreview').then((response) => { | ||
| expect(response.body).to.have.property('name', 'preview mode') | ||
| }) | ||
|
|
||
| // exptected content is rendered | ||
| cy.visit('/previewTest') | ||
| cy.findByText('Is preview? Yes!', { selector: 'h1' }) | ||
|
|
||
| // exit preview mode | ||
| cy.request('/api/exitPreview') | ||
| cy.visit('/previewTest') | ||
| cy.findByText('Is preview? No', { selector: 'h1' }) | ||
|
|
||
| // we should hit /previewTest 3 times (before entering preview, after entering preview, after exiting preview) | ||
| // this assertion is mainly to ensure interception works and assertion on response header is made | ||
| cy.get('@previewTestVisit.all').should('have.length', 3) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
It's good to have this like you mentioned and if we get around to running these tests on older versions, this will definitely be helpful.
| @@ -118,6 +119,13 @@ const getNetlifyNextServer = (NextServer: NextServerType) => { | |||
| } | |||
| } | |||
|
|
|||
| // eslint-disable-next-line class-methods-use-this, require-await | |||
| async runMiddleware(): Promise<{ finished: boolean }> { | |||
There was a problem hiding this comment.
Just linking to Next.js server's implementation for reference. https://github.com/vercel/next.js/blob/1286e145b058538e47b11cc1ec813a686cee1f68/packages/next/src/server/next-server.ts#L1474-L1589
pieh
added
automerge
and removed
type: chore
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
This change make
next-server'srunMiddlewareno-op in lambdas. There are some scenarios wherenext-serverwould attempt that (example one seems to be rendering page in preview mode). The middleware is executed at the edge so executing it at lambda would cause:@netlify/nextenhanced middleware as that would execute in Nodejs env which cause early bail because it's not Deno env..next/server/edge-runtime-webpack.jsand middleware module itself) - we could bundle those but because of all of above it seems better to not even try to run middleware at lambda.Making
runMiddlewarealways no-op in lambda seems safe for now as Next middleware is supposed to run only at edge, but vercel/next.js#46722 is a feature request asking to make middleware environment configurable, but at least as of now there was no reply from Next.js maintainersnext-serverin general does have capability to run in Nodejs env as that's what is used locally (next start) through Next.js sandbox environment ( https://github.com/vercel/next.js/tree/canary/packages/next/src/server/web/sandbox )It does seem like there is some difference in behavior between recent next versions (I could reproduce missing module problem with
[email protected], but it didn't seem to happen with[email protected]), but even if things don't crash, it still seems valid to not run middleware in lambda ever if it runs at edge already.Documentation
Tests
Added cypress tests for middleware demo site
Relevant links (GitHub issues, etc.) or a picture of cute animal
https://github.com/netlify/pod-ecosystem-frameworks/issues/588