Skip to content
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

Merged
merged 4 commits into from Apr 16, 2022

Conversation

SukkaW
Copy link
Contributor

@SukkaW SukkaW commented Apr 7, 2022

  • Make sure the linting passes by running 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.

@SukkaW SukkaW force-pushed the disable-nullish-optional-chain-transform branch from c058735 to f08683b Compare April 7, 2022 17:11
@ijjk

This comment has been minimized.

@ijjk
Copy link
Member

ijjk commented Apr 10, 2022

Stats from current PR

Default Build (Decrease detected ✓)
General Overall decrease ✓
vercel/next.js canary SukkaW/next.js disable-nullish-optional-chain-transform Change
buildDuration 20s 19.7s -319ms
buildDurationCached 7.6s 7.5s -37ms
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.22 4.271 ⚠️ +0.05
/ avg req/sec 592.48 585.38 ⚠️ -7.1
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 2.052 2.108 ⚠️ +0.06
/error-in-render avg req/sec 1218.59 1185.8 ⚠️ -32.79
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 ⚠️ +171ms
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 ⚠️ +0.04
/ avg req/sec 589.85 584.6 ⚠️ -5.25
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 2.058 2.063 ⚠️ +0.01
/error-in-render avg req/sec 1214.86 1211.69 ⚠️ -3.17
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
Commit: b028d7e

Copy link
Member

@timneutkens timneutkens left a 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.

@SukkaW
Copy link
Contributor Author

SukkaW commented Apr 15, 2022

Is optional chaining still downleveled when the targeted level doesn't support it? If that's the case these changes look fine.

@timneutkens Check test/integration/optional-chaining-nullish-coalescing. If that integrated test case passed, then it is fine.

@SukkaW SukkaW requested a review from timneutkens April 15, 2022 14:52
Copy link
Member

@ijjk ijjk left a 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

@SukkaW
Copy link
Contributor Author

SukkaW commented Apr 15, 2022

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

@ijjk

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.

@SukkaW SukkaW requested a review from ijjk April 15, 2022 16:20
@ijjk ijjk merged commit 3ae571d into vercel:canary Apr 16, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 16, 2022
@SukkaW SukkaW deleted the disable-nullish-optional-chain-transform branch October 24, 2023 08:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants