- 
                Notifications
    You must be signed in to change notification settings 
- Fork 178
Fix and enhancement for Next 16 #1029
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
base: main
Are you sure you want to change the base?
Conversation
| 🦋 Changeset detectedLatest commit: d017a4a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
 Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR | 
| commit:  | 
| Coverage Report
 File Coverage
 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Do we have to handle the  | 
| 
 Nope it doesn't change the name of the compiled file, but we have to update to Node 20 for e2e it seems.... | 
|  | ||
| const imageUpstream = isAbsolute | ||
| ? await fetchExternalImage(href) | ||
| ? //@ts-ignore | 
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.
Is this necessary? If so do an expect-error with description.
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.
Looks like it's related to #1026 (the signature changed with Next 16).
+1 to add a comment about that
| // size config to also set the ImageResponse's width and height. | ||
| ...size, | ||
| headers: { | ||
| "cache-control": "public, immutable, no-transform, max-age=31536000", | 
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.
Why did you set this?
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.
Next does not automatically set the cache on this anymore. Need to test with next start
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.
Very good job Nico! It seems to pass the E2E locally for me on Next 16.
Just a few comments and also could you rename the middleware files to proxy in all the examples app. (npx @next/codemod@canary middleware-to-proxy .)
To make the E2E work both locally and deployed we need to change this to 20:
| default: 18.x | 
And all the Runtime.NODEJS_18_X into Runtime.NODEJS_20_X in OpenNextReferenceImplementation.ts.
Also update package.json here:
Line 28 in 9b7f726
| "node": ">=18", | 
Perhaps remove these lines as we already setup node and pnpm cache in the setup/action.yml :
opennextjs-aws/.github/workflows/e2e.yml
Lines 95 to 99 in 9b7f726
| - name: Set up NodeJS v18 | |
| uses: actions/setup-node@v4 | |
| with: | |
| cache: pnpm # cache pnpm store | |
| node-version: 18.18.2 | 
However, we might do this in a seperate PR. Up to you.
| cacheFileMeta && | ||
| Array.isArray(cacheFileMeta.segmentPaths) && | 
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.
| cacheFileMeta && | |
| Array.isArray(cacheFileMeta.segmentPaths) && | |
| Array.isArray(cacheFileMeta?.segmentPaths) && | 
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.
cacheFileMeta.segmentPaths.length > 0 could also be skipped maybe
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.
Maybe something like:
const segments: Record<string, string> = Array.isArray(cacheFileMeta?.segmentPaths) ? 
  Object.fromEntries(cacheFileMeta.segmentPaths.map(...)) :
  {};
Should fix most build failures in Next 16.
Support for incremental prefetching (Fix #1022)
Support for incremental prefetching in the cache interceptor
Fix #1026