Skip to content

drop dynamic import with ssr: false on server-side#32606

Merged
timneutkens merged 3 commits intocanaryfrom
perf/drop-nossr-dynamic-import-on-server
Jan 11, 2022
Merged

drop dynamic import with ssr: false on server-side#32606
timneutkens merged 3 commits intocanaryfrom
perf/drop-nossr-dynamic-import-on-server

Conversation

@sokra
Copy link
Copy Markdown
Member

@sokra sokra commented Dec 17, 2021 •
edited by timneutkens
Loading

This drops the function passed to next/dynamic on server-side when ssr: false is statically found in the options object.

Improves build performance, reduces bundle size on the server-side and avoids touching client-only libraries at all on server-side.

Bug

  • Related issues linked using fixes #number
  • Integration tests added
  • Errors have helpful link attached, see contributing.md

Feature

  • Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • Related issues linked using fixes #number
  • Integration tests added
  • Documentation added
  • Telemetry added. In case of a feature if it's used or not.
  • Errors have helpful link attached, see contributing.md

Documentation / Examples

  • Make sure the linting passes by running yarn lint

@ijjk ijjk added created-by: Next.js team PRs by the Next.js team. type: next labels Dec 17, 2021
@ijjk

This comment has been minimized.

Sign in to view
@sokra sokra marked this pull request as ready for review December 17, 2021 16:38
@ijjk

This comment has been minimized.

Sign in to view
kdy1
kdy1 suggested changes Jan 10, 2022
View reviewed changes
Comment thread packages/next-swc/crates/core/src/next_dynamic.rs Outdated
{
for prop in options_props.iter() {
if let Some(KeyValueProp { key, value }) =
prop.clone().prop().and_then(|p| p.key_value())
Copy link
Copy Markdown
Contributor

@kdy1 kdy1 Jan 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will clone prop even if it's not required.

Copy link
Copy Markdown
Contributor

@kdy1 kdy1 Jan 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do

if let Some(KeyValueProp { key, value }) =
    prop.as_ref().and_then(|prop| match prop {
        PropOrSpread::Prop(prop) => match &**prop {
            Prop::KeyValue(kv) => Some(kv),
            _ => None,
        }
        _ => None,
    })
{
}

instead

Copy link
Copy Markdown
Contributor

@kdy1 kdy1 Jan 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With nightly-only feature box_syntax,

You can do

if let PropOrSpread::Prop(box Prop::KeyValue(KeyValueProp { key, value })) = prop
{
}

but this is nightly-only.

Copy link
Copy Markdown
Member Author

@sokra sokra Jan 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You seem to generate these enum accessors for the ast nodes, e. g.

pub fn key_value(self) -> Option<KeyValueProp>
// Returns Some if self is of variant KeyValue, and None otherwise.

In wonder why there isn't a ref variant to this, e. g.

pub fn key_value_ref(&self) -> Option<&KeyValueProp>
// Returns Some if self is of variant KeyValue, and None otherwise.

Copy link
Copy Markdown
Member Author

@sokra sokra Jan 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to write this nested if let match stuff as:

if let Some(KeyValueProp { key, value }) =
  try { prop.prop_ref()?.key_value_ref()? }
{

or with Option::and_then instead of try blocks^^

Copy link
Copy Markdown
Contributor

@kdy1 kdy1 Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For ref-based methods, I think it's good to have one. I'll implement it soon.

Copy link
Copy Markdown
Contributor

@kdy1 kdy1 Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. swc-project/swc#3226

It can't be used with current code, because ast type definitions are modified recently.

You can rebase and use methods related to ref after I make a PR to update swc crates, or modify code after merging this PR.
Which do you prefer?
If you prefer the former, I'll make a upgrade PR asap.

Copy link
Copy Markdown
Member Author

@sokra sokra Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be improved in follow up PRs.

Comment thread packages/next-swc/crates/core/src/next_dynamic.rs Outdated
Comment thread packages/next-swc/crates/core/src/next_dynamic.rs Outdated
Comment thread packages/next-swc/crates/core/src/next_dynamic.rs Outdated
@sokra sokra force-pushed the perf/drop-nossr-dynamic-import-on-server branch from ce27a78 to b22fdc4 Compare January 10, 2022 14:50
@ijjk

This comment has been minimized.

Sign in to view
kdy1
kdy1 approved these changes Jan 11, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. NIce work!

@ijjk
Copy link
Copy Markdown
Member

ijjk commented Jan 11, 2022

Stats from current PR

Default Build (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js perf/drop-nossr-dynamic-import-on-server Change
buildDuration 16.8s 16.7s -39ms
buildDurationCached 3.7s 3.7s ⚠️ +89ms
nodeModulesSize 355 MB 355 MB ⚠️ +7 B
Page Load Tests Overall decrease ⚠️
vercel/next.js canary vercel/next.js perf/drop-nossr-dynamic-import-on-server Change
/ failed reqs 0 0 ✓
/ total time (seconds) 3.555 3.601 ⚠️ +0.05
/ avg req/sec 703.3 694.28 ⚠️ -9.02
/error-in-render failed reqs 0 0 ✓
/error-in-render total time (seconds) 1.81 1.857 ⚠️ +0.05
/error-in-render avg req/sec 1381.47 1346.09 ⚠️ -35.38
Client Bundles (main, webpack, commons)
vercel/next.js canary vercel/next.js perf/drop-nossr-dynamic-import-on-server Change
450.HASH.js gzip 179 B 179 B ✓
framework-HASH.js gzip 42.2 kB 42.2 kB ✓
main-HASH.js gzip 27.1 kB 27.1 kB ✓
webpack-HASH.js gzip 1.45 kB 1.45 kB ✓
Overall change 70.9 kB 70.9 kB ✓
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js perf/drop-nossr-dynamic-import-on-server Change
polyfills-HASH.js gzip 31 kB 31 kB ✓
Overall change 31 kB 31 kB ✓
Client Pages
vercel/next.js canary vercel/next.js perf/drop-nossr-dynamic-import-on-server Change
_app-HASH.js gzip 1.37 kB 1.37 kB ✓
_error-HASH.js gzip 194 B 194 B ✓
amp-HASH.js gzip 312 B 312 B ✓
css-HASH.js gzip 326 B 326 B ✓
dynamic-HASH.js gzip 2.37 kB 2.37 kB ✓
head-HASH.js gzip 350 B 350 B ✓
hooks-HASH.js gzip 919 B 919 B ✓
image-HASH.js gzip 4.74 kB 4.74 kB ✓
index-HASH.js gzip 263 B 263 B ✓
link-HASH.js gzip 2.13 kB 2.13 kB ✓
routerDirect..HASH.js gzip 321 B 321 B ✓
script-HASH.js gzip 383 B 383 B ✓
withRouter-HASH.js gzip 318 B 318 B ✓
85e02e95b279..7e3.css gzip 107 B 107 B ✓
Overall change 14.1 kB 14.1 kB ✓
Client Build Manifests
vercel/next.js canary vercel/next.js perf/drop-nossr-dynamic-import-on-server Change
_buildManifest.js gzip 459 B 459 B ✓
Overall change 459 B 459 B ✓
Rendered Page Sizes
vercel/next.js canary vercel/next.js perf/drop-nossr-dynamic-import-on-server Change
index.html gzip 533 B 533 B ✓
link.html gzip 547 B 547 B ✓
withRouter.html gzip 528 B 528 B ✓
Overall change 1.61 kB 1.61 kB ✓
Default Build with SWC (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js perf/drop-nossr-dynamic-import-on-server Change
buildDuration 18.8s 18.6s -192ms
buildDurationCached 3.8s 3.7s -38ms
nodeModulesSize 355 MB 355 MB ⚠️ +7 B
Page Load Tests Overall increase ✓
vercel/next.js canary vercel/next.js perf/drop-nossr-dynamic-import-on-server Change
/ failed reqs 0 0 ✓
/ total time (seconds) 3.676 3.598 -0.08
/ avg req/sec 680.13 694.76 +14.63
/error-in-render failed reqs 0 0 ✓
/error-in-render total time (seconds) 2.062 1.851 -0.21
/error-in-render avg req/sec 1212.36 1350.27 +137.91
Client Bundles (main, webpack, commons)
vercel/next.js canary vercel/next.js perf/drop-nossr-dynamic-import-on-server Change
450.HASH.js gzip 179 B 179 B ✓
framework-HASH.js gzip 42.3 kB 42.3 kB ✓
main-HASH.js gzip 27.2 kB 27.2 kB ✓
webpack-HASH.js gzip 1.44 kB 1.44 kB ✓
Overall change 71.1 kB 71.1 kB ✓
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js perf/drop-nossr-dynamic-import-on-server Change
polyfills-HASH.js gzip 31 kB 31 kB ✓
Overall change 31 kB 31 kB ✓
Client Pages
vercel/next.js canary vercel/next.js perf/drop-nossr-dynamic-import-on-server Change
_app-HASH.js gzip 1.35 kB 1.35 kB ✓
_error-HASH.js gzip 180 B 180 B ✓
amp-HASH.js gzip 305 B 305 B ✓
css-HASH.js gzip 321 B 321 B ✓
dynamic-HASH.js gzip 2.36 kB 2.36 kB ✓
head-HASH.js gzip 342 B 342 B ✓
hooks-HASH.js gzip 906 B 906 B ✓
image-HASH.js gzip 4.76 kB 4.76 kB ✓
index-HASH.js gzip 256 B 256 B ✓
link-HASH.js gzip 2.19 kB 2.19 kB ✓
routerDirect..HASH.js gzip 314 B 314 B ✓
script-HASH.js gzip 375 B 375 B ✓
withRouter-HASH.js gzip 309 B 309 B ✓
85e02e95b279..7e3.css gzip 107 B 107 B ✓
Overall change 14.1 kB 14.1 kB ✓
Client Build Manifests
vercel/next.js canary vercel/next.js perf/drop-nossr-dynamic-import-on-server Change
_buildManifest.js gzip 458 B 458 B ✓
Overall change 458 B 458 B ✓
Rendered Page Sizes
vercel/next.js canary vercel/next.js perf/drop-nossr-dynamic-import-on-server 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: 71f2aed

@timneutkens timneutkens merged commit 9836429 into canary Jan 11, 2022
@timneutkens timneutkens deleted the perf/drop-nossr-dynamic-import-on-server branch January 11, 2022 13:46
teleaziz added a commit to teleaziz/next.js that referenced this pull request Jan 11, 2022
@teleaziz
Merge branch 'cms-builder-io-example' of github.com:teleaziz/next.js …
ce988af 
…into cms-builder-io-example

* 'cms-builder-io-example' of github.com:teleaziz/next.js: (22 commits)
  drop dynamic import with `ssr: false` on server-side (vercel#32606)
  next-swc: fix ssg code elimination when used in render (vercel#32709)
  Add Caveats section to custom error page (vercel#33160)
  v12.0.8-canary.20
  Allow dependencies to use environment variables in middlewares (vercel#33141)
  use a separate webpack runtime for middleware (vercel#33134)
  No info on environment variables in the src folder (vercel#33110) (vercel#33136)
  docs: minor text-copy cleanup (vercel#33120)
  Update swc (vercel#33063)
  Update next.config.js (vercel#33091)
  Adding Asset Component for Rich Text Renderer (vercel#32503)
  Update using-mdx.md (vercel#33077)
  v12.0.8-canary.19
  Fix middleware at root in standalone mode (vercel#33053)
  Add util for generating new tests/error documents (vercel#33001)
  Remove extra config from tailwind example (vercel#33062)
  Fix link for Next.js Analytics in docs (vercel#33049)
  Bump `@vercel/nft` to 0.17.2 (vercel#33048)
  Update deployment documentation. (vercel#32006)
  v12.0.8-canary.18
  ...
@vercel vercel locked as resolved and limited conversation to collaborators Feb 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@huozhi huozhi Awaiting requested review from huozhi

@ijjk ijjk Awaiting requested review from ijjk

@shuding shuding Awaiting requested review from shuding

@timneutkens timneutkens Awaiting requested review from timneutkens

1 more reviewer

@kdy1 kdy1 kdy1 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

created-by: Next.js team PRs by the Next.js team. type: next

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sokra @ijjk @kdy1 @timneutkens