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

Replace optionalDependencies with script for installing swc #33496

Closed
wants to merge 2 commits into from

Conversation

ijjk
Copy link
Member

@ijjk ijjk commented Jan 20, 2022

This continues the work in #32850 to reduce install size/time for the next-swc binary. Since optionalDependencies don't allow us to accurately download the specific binary we need per-platform since platforms can match arch and platform but still need a different binary e.g. musl vs glibc for linux this adds a custom install script which will directly download the correct binary.

Initial testing seems to reduce install time by 2 -3 seconds on mac OS and Windows even without the local cache for the next-swc binary being leveraged. We can expand on the install script in the future to download the WASM variant of next-swc if a platform specific binary is not available yet.

before (yarn cache was cleared before run)
yarn add next@/Users/ijjk/dev/vercel/next.js/packages/next/next-v12.0.9-canary.3-optional.tgz
yarn add v1.22.17
info No lockfile found.
[1/4] πŸ”  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] πŸ”—  Linking dependencies...
warning " > next@12.0.9-canary.3" has unmet peer dependency "react@^17.0.2 || ^18.0.0-0".
warning " > next@12.0.9-canary.3" has unmet peer dependency "react-dom@^17.0.2 || ^18.0.0-0".
warning "next > styled-jsx@5.0.0-beta.7" has unmet peer dependency "react@>= 16.8.0 || 17.x.x || 18.x.x".
warning "next > use-subscription@1.5.1" has unmet peer dependency "react@^16.8.0 || ^17.0.0".
[4/4] πŸ”¨  Building fresh packages...
success Saved lockfile.
success Saved 11 new dependencies.
info Direct dependencies
└─ next@12.0.9-canary.3
info All dependencies
β”œβ”€ @next/env@12.0.9-canary.3
β”œβ”€ @next/swc-darwin-x64@12.0.9-canary.3
β”œβ”€ caniuse-lite@1.0.30001300
β”œβ”€ nanoid@3.2.0
β”œβ”€ next@12.0.9-canary.3
β”œβ”€ object-assign@4.1.1
β”œβ”€ picocolors@1.0.0
β”œβ”€ postcss@8.4.5
β”œβ”€ source-map-js@1.0.2
β”œβ”€ styled-jsx@5.0.0-beta.7
└─ use-subscription@1.5.1
✨  Done in 6.96s.
after (yarn cache was cleared before run)
yarn add next@/Users/ijjk/dev/vercel/next.js/packages/next/next-v12.0.9-canary.3-install.tgz
yarn add v1.22.17
info No lockfile found.
[1/4] πŸ”  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] πŸ”—  Linking dependencies...
warning " > next@12.0.9-canary.3" has unmet peer dependency "react@^17.0.2 || ^18.0.0-0".
warning " > next@12.0.9-canary.3" has unmet peer dependency "react-dom@^17.0.2 || ^18.0.0-0".
warning "next > styled-jsx@5.0.0-beta.7" has unmet peer dependency "react@>= 16.8.0 || 17.x.x || 18.x.x".
warning "next > use-subscription@1.5.1" has unmet peer dependency "react@^16.8.0 || ^17.0.0".
[4/4] πŸ”¨  Building fresh packages...
success Saved lockfile.
success Saved 10 new dependencies.
info Direct dependencies
└─ next@12.0.9-canary.3
info All dependencies
β”œβ”€ @next/env@12.0.9-canary.3
β”œβ”€ caniuse-lite@1.0.30001300
β”œβ”€ nanoid@3.2.0
β”œβ”€ next@12.0.9-canary.3
β”œβ”€ object-assign@4.1.1
β”œβ”€ picocolors@1.0.0
β”œβ”€ postcss@8.4.5
β”œβ”€ source-map-js@1.0.2
β”œβ”€ styled-jsx@5.0.0-beta.7
└─ use-subscription@1.5.1
✨  Done in 3.69s.

@ijjk ijjk added created-by: Next.js team PRs by the Next.js team type: next labels Jan 20, 2022
@ijjk

This comment has been minimized.

@Brooooooklyn
Copy link
Contributor

Yarn berry has started working on the libc field in the package.json yarnpkg/berry#3981

@ijjk ijjk marked this pull request as ready for review January 20, 2022 15:58
@ijjk ijjk requested a review from styfle January 20, 2022 15:58
@ijjk
Copy link
Member Author

ijjk commented Jan 20, 2022

@Brooooooklyn I'm curious if optionalDependencies is the right solution here even if the musl/glib issue is fixed in all relevant package managers as it still requires resolving all of the package data for each optionalDependency from the registry even though we can know earlier if the package won't be needed for specific platforms.


if (!activePackage) {
// TODO: should this be a hard error even though it fails install?
// should we fallback to wasm in this case?
Copy link
Member

Choose a reason for hiding this comment

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

Falling back to wasm makes sense to me.

Thats the advantage of this custom install script πŸ‘

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, falling back to wasm sounds good to me.


try {
await fetch(
`https://registry.npmjs.org/${activePackage.packageName}/-/${tarFileName}`
Copy link
Contributor

@merceyz merceyz Jan 20, 2022

Choose a reason for hiding this comment

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

Some setups don't have access to the npm registry so this will be problematic
Ref evanw/esbuild#1621

Copy link
Member

@styfle styfle Jan 21, 2022

Choose a reason for hiding this comment

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

Yeah this is problematic because package config like .npmrc or .yarnrc is no longer respected evanw/esbuild#286

Perhaps we need to double down on getting this feature into npm now that yarn just landed it in yarnpkg/berry#3981

Copy link
Member Author

Choose a reason for hiding this comment

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

We can definitely handle loading the registry from the package config. I think it's gonna be a while before this is sorted out/performant in relevant package managers so the custom install script will still be needed for a bit.

@Brooooooklyn
Copy link
Contributor

I'm curious if optionalDependencies is the right solution here even if the musl/glib issue is fixed in all relevant package managers as it still requires resolving all of the package data for each optionalDependency from the registry even though we can know earlier if the package won't be needed for specific platforms.

Yes, resolving extra meta info from the registry is a massive problem in the optionalDependencies strategy.

We are discussing some other solutions for native packages npm/rfcs#438 (comment)

Co-authored-by: Steven <steven@ceriously.com>
break
} catch (e) {}
}

if (!bindings) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this block any more?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we should for most cases although we could leave it in case users want to manually install the package and skip our custom install script πŸ€”

@ijjk
Copy link
Member Author

ijjk commented Jan 20, 2022

Stats from current PR

Default Build (Decrease detected βœ“)
General Overall increase ⚠️
vercel/next.js canary ijjk/next.js update/swc-downloading Change
buildDuration 18.5s 18.8s ⚠️ +294ms
buildDurationCached 4.1s 4.2s ⚠️ +40ms
nodeModulesSize 355 MB 355 MB ⚠️ +102 kB
Page Load Tests Overall decrease ⚠️
vercel/next.js canary ijjk/next.js update/swc-downloading Change
/ failed reqs 0 0 βœ“
/ total time (seconds) 3.995 3.996 0
/ avg req/sec 625.77 625.62 ⚠️ -0.15
/error-in-render failed reqs 0 0 βœ“
/error-in-render total time (seconds) 2.067 2.189 ⚠️ +0.12
/error-in-render avg req/sec 1209.71 1141.99 ⚠️ -67.72
Client Bundles (main, webpack, commons)
vercel/next.js canary ijjk/next.js update/swc-downloading Change
450.HASH.js gzip 179 B 179 B βœ“
framework-HASH.js gzip 42.2 kB 42.2 kB βœ“
main-HASH.js gzip 27.2 kB 27.2 kB βœ“
webpack-HASH.js gzip 1.44 kB 1.44 kB βœ“
Overall change 71 kB 71 kB βœ“
Legacy Client Bundles (polyfills)
vercel/next.js canary ijjk/next.js update/swc-downloading Change
polyfills-HASH.js gzip 31 kB 31 kB βœ“
Overall change 31 kB 31 kB βœ“
Client Pages
vercel/next.js canary ijjk/next.js update/swc-downloading 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.87 kB 4.87 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.2 kB 14.2 kB βœ“
Client Build Manifests
vercel/next.js canary ijjk/next.js update/swc-downloading Change
_buildManifest.js gzip 459 B 459 B βœ“
Overall change 459 B 459 B βœ“
Rendered Page Sizes
vercel/next.js canary ijjk/next.js update/swc-downloading Change
index.html gzip 531 B 531 B βœ“
link.html gzip 545 B 545 B βœ“
withRouter.html gzip 526 B 526 B βœ“
Overall change 1.6 kB 1.6 kB βœ“

Default Build with SWC (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary ijjk/next.js update/swc-downloading Change
buildDuration 23.3s 23.5s ⚠️ +195ms
buildDurationCached 4.2s 4.1s -34ms
nodeModulesSize 355 MB 355 MB ⚠️ +102 kB
Page Load Tests Overall increase βœ“
vercel/next.js canary ijjk/next.js update/swc-downloading Change
/ failed reqs 0 0 βœ“
/ total time (seconds) 4 4.061 ⚠️ +0.06
/ avg req/sec 624.94 615.55 ⚠️ -9.39
/error-in-render failed reqs 0 0 βœ“
/error-in-render total time (seconds) 2.159 2.117 -0.04
/error-in-render avg req/sec 1157.86 1180.64 +22.78
Client Bundles (main, webpack, commons)
vercel/next.js canary ijjk/next.js update/swc-downloading Change
450.HASH.js gzip 179 B 179 B βœ“
framework-HASH.js gzip 42.3 kB 42.3 kB βœ“
main-HASH.js gzip 27.3 kB 27.3 kB βœ“
webpack-HASH.js gzip 1.44 kB 1.44 kB βœ“
Overall change 71.3 kB 71.3 kB βœ“
Legacy Client Bundles (polyfills)
vercel/next.js canary ijjk/next.js update/swc-downloading Change
polyfills-HASH.js gzip 31 kB 31 kB βœ“
Overall change 31 kB 31 kB βœ“
Client Pages
vercel/next.js canary ijjk/next.js update/swc-downloading 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 911 B 911 B βœ“
image-HASH.js gzip 4.88 kB 4.88 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.2 kB 14.2 kB βœ“
Client Build Manifests
vercel/next.js canary ijjk/next.js update/swc-downloading Change
_buildManifest.js gzip 459 B 459 B βœ“
Overall change 459 B 459 B βœ“
Rendered Page Sizes
vercel/next.js canary ijjk/next.js update/swc-downloading 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: 54bd12a

// should we fallback to wasm in this case?
console.error(
`Error: unsupported next-swc platform: ` +
`${process.platform} ${process.arch} (glibc: ${isGlibc})\n` +
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`${process.platform} ${process.arch} (glibc: ${isGlibc})\n` +
`${process.platform} ${process.arch} (glibcVersionRuntime: ${glibcVersionRuntime})\n` +

kodiakhq bot pushed a commit that referenced this pull request May 2, 2022
Follow-up to #36527 this adds falling back to the wasm swc build when loading the native bindings fails so that we don't block the build on the native dependency being available.  

This continues off of #33496 but does not add a postinstall script yet and only downloads the fallback when the native dependency fails to load.
Copy link
Member Author

@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.

Going to close this for now as we have wasm fallback handling now and taking separate steps to refine this installation process.

@ijjk ijjk closed this Jul 26, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants