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
refactor(build): no force transpile optional chaining #35976
refactor(build): no force transpile optional chaining #35976
Conversation
c058735
to
f08683b
Compare
This comment has been minimized.
This comment has been minimized.
Stats from current PRDefault Build (Decrease detected ✓)General Overall decrease ✓
Page Load Tests Overall decrease
|
vercel/next.js canary | SukkaW/next.js disable-nullish-optional-chain-transform | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 4.22 | 4.271 | |
/ avg req/sec | 592.48 | 585.38 | |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 2.052 | 2.108 | |
/error-in-render avg req/sec | 1218.59 | 1185.8 |
Client Bundles (main, webpack)
vercel/next.js canary | SukkaW/next.js disable-nullish-optional-chain-transform | Change | |
---|---|---|---|
925.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42 kB | 42 kB | ✓ |
main-HASH.js gzip | 28.3 kB | 28.3 kB | ✓ |
webpack-HASH.js gzip | 1.44 kB | 1.44 kB | ✓ |
Overall change | 72 kB | 72 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | SukkaW/next.js disable-nullish-optional-chain-transform | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | SukkaW/next.js disable-nullish-optional-chain-transform | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.36 kB | 1.36 kB | ✓ |
_error-HASH.js gzip | 192 B | 192 B | ✓ |
amp-HASH.js gzip | 309 B | 309 B | ✓ |
css-HASH.js gzip | 327 B | 327 B | ✓ |
dynamic-HASH.js gzip | 3.04 kB | 3.04 kB | ✓ |
head-HASH.js gzip | 351 B | 351 B | ✓ |
hooks-HASH.js gzip | 920 B | 920 B | ✓ |
image-HASH.js gzip | 5.74 kB | 5.74 kB | ✓ |
index-HASH.js gzip | 263 B | 263 B | ✓ |
link-HASH.js gzip | 2.36 kB | 2.36 kB | ✓ |
routerDirect..HASH.js gzip | 320 B | 320 B | ✓ |
script-HASH.js gzip | 392 B | 392 B | ✓ |
withRouter-HASH.js gzip | 319 B | 319 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 16 kB | 16 kB | ✓ |
Client Build Manifests
vercel/next.js canary | SukkaW/next.js disable-nullish-optional-chain-transform | Change | |
---|---|---|---|
_buildManifest.js gzip | 461 B | 461 B | ✓ |
Overall change | 461 B | 461 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | SukkaW/next.js disable-nullish-optional-chain-transform | Change | |
---|---|---|---|
index.html gzip | 530 B | 530 B | ✓ |
link.html gzip | 544 B | 544 B | ✓ |
withRouter.html gzip | 524 B | 524 B | ✓ |
Overall change | 1.6 kB | 1.6 kB | ✓ |
Default Build with SWC (Decrease detected ✓)
General Overall decrease ✓
vercel/next.js canary | SukkaW/next.js disable-nullish-optional-chain-transform | Change | |
---|---|---|---|
buildDuration | 22.9s | 23.1s | |
buildDurationCached | 7.5s | 7.4s | -88ms |
nodeModulesSize | 485 MB | 485 MB | -1.17 kB |
Page Load Tests Overall decrease ⚠️
vercel/next.js canary | SukkaW/next.js disable-nullish-optional-chain-transform | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 4.238 | 4.276 | |
/ avg req/sec | 589.85 | 584.6 | |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 2.058 | 2.063 | |
/error-in-render avg req/sec | 1214.86 | 1211.69 |
Client Bundles (main, webpack)
vercel/next.js canary | SukkaW/next.js disable-nullish-optional-chain-transform | Change | |
---|---|---|---|
925.HASH.js gzip | 178 B | 178 B | ✓ |
framework-HASH.js gzip | 42.3 kB | 42.3 kB | ✓ |
main-HASH.js gzip | 28.7 kB | 28.7 kB | ✓ |
webpack-HASH.js gzip | 1.45 kB | 1.45 kB | ✓ |
Overall change | 72.6 kB | 72.6 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | SukkaW/next.js disable-nullish-optional-chain-transform | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | SukkaW/next.js disable-nullish-optional-chain-transform | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.35 kB | 1.35 kB | ✓ |
_error-HASH.js gzip | 179 B | 179 B | ✓ |
amp-HASH.js gzip | 313 B | 313 B | ✓ |
css-HASH.js gzip | 325 B | 325 B | ✓ |
dynamic-HASH.js gzip | 3.02 kB | 3.02 kB | ✓ |
head-HASH.js gzip | 351 B | 351 B | ✓ |
hooks-HASH.js gzip | 921 B | 921 B | ✓ |
image-HASH.js gzip | 5.78 kB | 5.78 kB | ✓ |
index-HASH.js gzip | 261 B | 261 B | ✓ |
link-HASH.js gzip | 2.44 kB | 2.44 kB | ✓ |
routerDirect..HASH.js gzip | 322 B | 322 B | ✓ |
script-HASH.js gzip | 393 B | 393 B | ✓ |
withRouter-HASH.js gzip | 317 B | 317 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 16.1 kB | 16.1 kB | ✓ |
Client Build Manifests
vercel/next.js canary | SukkaW/next.js disable-nullish-optional-chain-transform | Change | |
---|---|---|---|
_buildManifest.js gzip | 457 B | 457 B | ✓ |
Overall change | 457 B | 457 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | SukkaW/next.js disable-nullish-optional-chain-transform | Change | |
---|---|---|---|
index.html gzip | 532 B | 532 B | ✓ |
link.html gzip | 545 B | 545 B | ✓ |
withRouter.html gzip | 526 B | 526 B | ✓ |
Overall change | 1.6 kB | 1.6 kB | ✓ |
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 optional chaining still downleveled when the targeted level doesn't support it? If that's the case these changes look fine.
@timneutkens Check |
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.
This change looks alright now as the test added in #33995 is passing against Node.js v16 and v17 which is what the affected versions were. I think it would be good to keep the unit tests though
Next.js 12 requires a minimum Node.js version of 12.22.0, which doesn't support optional chaining syntax. And the CI runs tests on Node.js 16 & 17 which both have optional chaining syntax support. Thus the emitted code will be inconsistent against different Node.js versions. IMHO the behavior should be tested by swc and babel (whether they can identify and apply transpilation), not by Next.js. |
yarn lint
Ref:
Previously, webpack has some serious issues processing optional chaining (webpack/webpack#10227 which is fixed in webpack 5, and webpack/webpack#12960 which is fixed in webpack 5.71.0). The issues are also being reported to Next.js (#17273 which is fixed by #17429, and #33915 which is fixed by #33995).
With Next.js has deprecated Webpack 4 since this major version and has updated Webpack to 5.71.0 (#35867), it is now safe to remove force transpilation of optional chaining and nullish operator.