-
Notifications
You must be signed in to change notification settings - Fork 27.5k
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: always ensure element before set to weakmap #75012
Conversation
Stats from current PRDefault Build (Increase detected
|
vercel/next.js canary | vercel/next.js huozhi/fix-forward | Change | |
---|---|---|---|
buildDuration | 18.3s | 15.5s | N/A |
buildDurationCached | 14.5s | 12.3s | N/A |
nodeModulesSize | 418 MB | 418 MB | |
nextStartRea..uration (ms) | 396ms | 394ms | N/A |
Client Bundles (main, webpack)
vercel/next.js canary | vercel/next.js huozhi/fix-forward | Change | |
---|---|---|---|
5306-HASH.js gzip | 54 kB | 54 kB | N/A |
8276.HASH.js gzip | 169 B | 168 B | N/A |
8377-HASH.js gzip | 5.44 kB | 5.44 kB | N/A |
bccd1874-HASH.js gzip | 52.9 kB | 52.9 kB | ✓ |
framework-HASH.js gzip | 57.5 kB | 57.5 kB | N/A |
main-app-HASH.js gzip | 240 B | 242 B | N/A |
main-HASH.js gzip | 34.3 kB | 34.3 kB | N/A |
webpack-HASH.js gzip | 1.71 kB | 1.71 kB | N/A |
Overall change | 52.9 kB | 52.9 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | vercel/next.js huozhi/fix-forward | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 39.4 kB | 39.4 kB | ✓ |
Overall change | 39.4 kB | 39.4 kB | ✓ |
Client Pages
vercel/next.js canary | vercel/next.js huozhi/fix-forward | Change | |
---|---|---|---|
_app-HASH.js gzip | 193 B | 193 B | ✓ |
_error-HASH.js gzip | 193 B | 193 B | ✓ |
amp-HASH.js gzip | 512 B | 510 B | N/A |
css-HASH.js gzip | 343 B | 342 B | N/A |
dynamic-HASH.js gzip | 1.84 kB | 1.84 kB | ✓ |
edge-ssr-HASH.js gzip | 265 B | 265 B | ✓ |
head-HASH.js gzip | 363 B | 362 B | N/A |
hooks-HASH.js gzip | 393 B | 392 B | N/A |
image-HASH.js gzip | 4.57 kB | 4.57 kB | N/A |
index-HASH.js gzip | 268 B | 268 B | ✓ |
link-HASH.js gzip | 2.35 kB | 2.34 kB | N/A |
routerDirect..HASH.js gzip | 328 B | 328 B | ✓ |
script-HASH.js gzip | 397 B | 397 B | ✓ |
withRouter-HASH.js gzip | 323 B | 326 B | N/A |
1afbb74e6ecf..834.css gzip | 106 B | 106 B | ✓ |
Overall change | 3.59 kB | 3.59 kB | ✓ |
Client Build Manifests
vercel/next.js canary | vercel/next.js huozhi/fix-forward | Change | |
---|---|---|---|
_buildManifest.js gzip | 749 B | 747 B | N/A |
Overall change | 0 B | 0 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | vercel/next.js huozhi/fix-forward | Change | |
---|---|---|---|
index.html gzip | 522 B | 524 B | N/A |
link.html gzip | 538 B | 537 B | N/A |
withRouter.html gzip | 519 B | 520 B | N/A |
Overall change | 0 B | 0 B | ✓ |
Edge SSR bundle Size
vercel/next.js canary | vercel/next.js huozhi/fix-forward | Change | |
---|---|---|---|
edge-ssr.js gzip | 129 kB | 129 kB | N/A |
page.js gzip | 208 kB | 208 kB | N/A |
Overall change | 0 B | 0 B | ✓ |
Middleware size
vercel/next.js canary | vercel/next.js huozhi/fix-forward | Change | |
---|---|---|---|
middleware-b..fest.js gzip | 669 B | 665 B | N/A |
middleware-r..fest.js gzip | 155 B | 156 B | N/A |
middleware.js gzip | 31.3 kB | 31.3 kB | N/A |
edge-runtime..pack.js gzip | 844 B | 844 B | ✓ |
Overall change | 844 B | 844 B | ✓ |
Next Runtimes
vercel/next.js canary | vercel/next.js huozhi/fix-forward | Change | |
---|---|---|---|
274-experime...dev.js gzip | 322 B | 322 B | ✓ |
274.runtime.dev.js gzip | 314 B | 314 B | ✓ |
app-page-exp...dev.js gzip | 374 kB | 374 kB | ✓ |
app-page-exp..prod.js gzip | 130 kB | 130 kB | ✓ |
app-page-tur..prod.js gzip | 143 kB | 143 kB | ✓ |
app-page-tur..prod.js gzip | 139 kB | 139 kB | ✓ |
app-page.run...dev.js gzip | 362 kB | 362 kB | ✓ |
app-page.run..prod.js gzip | 126 kB | 126 kB | ✓ |
app-route-ex...dev.js gzip | 37.6 kB | 37.6 kB | ✓ |
app-route-ex..prod.js gzip | 25.6 kB | 25.6 kB | ✓ |
app-route-tu..prod.js gzip | 25.6 kB | 25.6 kB | ✓ |
app-route-tu..prod.js gzip | 25.4 kB | 25.4 kB | ✓ |
app-route.ru...dev.js gzip | 39.2 kB | 39.2 kB | ✓ |
app-route.ru..prod.js gzip | 25.4 kB | 25.4 kB | ✓ |
pages-api-tu..prod.js gzip | 9.69 kB | 9.69 kB | ✓ |
pages-api.ru...dev.js gzip | 11.6 kB | 11.6 kB | ✓ |
pages-api.ru..prod.js gzip | 9.68 kB | 9.68 kB | ✓ |
pages-turbo...prod.js gzip | 21.8 kB | 21.8 kB | ✓ |
pages.runtim...dev.js gzip | 27.6 kB | 27.6 kB | ✓ |
pages.runtim..prod.js gzip | 21.8 kB | 21.8 kB | ✓ |
server.runti..prod.js gzip | 916 kB | 916 kB | ✓ |
Overall change | 2.47 MB | 2.47 MB | ✓ |
build cache Overall increase ⚠️
vercel/next.js canary | vercel/next.js huozhi/fix-forward | Change | |
---|---|---|---|
0.pack gzip | 2.1 MB | 2.1 MB | |
index.pack gzip | 74.7 kB | 74.3 kB | N/A |
Overall change | 2.1 MB | 2.1 MB |
Diff details
Diff for main-HASH.js
Diff too large to display
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.
Hmmm this doesn't look right. observeLinkVisibilityOnMount
returns a cleanup, so it should never receive null
(useMergedRef
should handle cleanups just fine, we updated it to do that some time ago, but maybe it's buggy?)
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 weird issue, shouldn't be possible, but LGTM to unblock
Updated the comment as after investigation it's from react feeding a null ref rather than the useMergeRef itself returning null |
This reverts commit 7229934.
This reverts commit 7229934.
This reverts commit 7229934.
- Fixes a crash when a Link unmounts (under some VERY specific conditions) - Reverts #75012, which implemented a workaround for the crash ### Short explanation The crash happens because we use a cleanup-returning ref in (the app-dir version of) Link, and `<Link legacyBehavior>` "leaks" that ref into user code. If user code does ref-merging incorrectly, i.e. it treats all callback refs as the old style that gets called with `null`, then our ref can get called with `null` even though it shouldn't, and crashes. Technically this crash is not a Next.js bug. But I think a lot user code uses ref-merging libraries that haven't been updated for 19, and can crash because of this. So i think the safest course of action is to be defensive, and change `useMergedRef` to always convert cleanup refs to normal function refs, so that userspace can't mess up our refs. ### Full explanation - `observeLinkVisibilityOnMount` inside our Link is a cleanup-returning ref - i.e. the new thing added in react 19, `(el: Element) => () => void` instead of the old `(el: Element | null) => void` - user code does `<Link legacyBehavior><CustomComponent /></Link>` - `legacyBehavior` means that props get spread onto the child, including `ref`, so we'll pass the ref created inside Link to `CustomComponent` - AND no `ref` is passed on the `<CustomComponent>` itself - with `legacyBehavior`, that'd get extracted and merged with `observeLinkVisibilityOnMount` - if there's no user ref, the old implementation of `useMergedRef` would return `observeLinkVisibilityOnMount` unchanged, thus passing `CustomComponent` a cleanup ref - `CustomComponent` uses a buggy ref merging library, i.e. one that wasn't updated to handle cleanup refs. - **so `observeLinkVisibilityOnMount` gets treated as an old-style callback ref, i.e. it gets called with`null`. and stuff explodes, because it doesn't expect that** The test in `test/production/next-link-legacybehavior-ref-merging` reproduces the crash. You can check out the commit that adds it to see it happen -- I put it before the fix.
What
Regression introduced in #74670, where the element somehow is invalid to be observed by observer or be set as key in to WeakMap. The possible cause is
mountInstance
is calling withnull
.The root cause is the
useMergedRef
could returnnull
in callback, we need to filter them out before passing tomountLinkInstance
orunmountLinkInstance