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.

Sign up for GitHub

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

Jump to bottom

Ensure conditional resolve alias does not affect server #29673

Merged
merged 3 commits into from Oct 6, 2021
Merged

Ensure conditional resolve alias does not affect server #29673

merged 3 commits into from Oct 6, 2021

Conversation

ijjk
Copy link
Member

@ijjk ijjk commented Oct 6, 2021

This ensures we don't add the no-op resolve alias when rewrites aren't used for the server resolving since it is still needed while tracing server files.

Bug

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

Fixes: #25538 (comment)

@ijjk ijjk added created-by: Next.js team PRs by the Next.js team type: next labels Oct 6, 2021
styfle
styfle reviewed Oct 6, 2021
View reviewed changes
test/integration/production/test/index.test.js Outdated
@@ -133,6 +133,7 @@ describe('Production Usage', () => {
/node_modules\/react\/cjs\/react\.production\.min\.js/,
/next\/router\.js/,
/next\/dist\/client\/router\.js/,
/next\/dist\/shared\/lib\/router\/utils\/resolve-rewrites\.js/,
Copy link
Member

Choose a reason for hiding this comment

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

Should this be added to a notTests for another page?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it should be included in all of the tested pages since they use next/router or next/link, I updated tests to reflect this in 6aeccbf

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we need a new page that doesn't use next/router or next/link

@ijjk ijjk mentioned this pull request Oct 6, 2021
Merged
2 tasks
styfle
styfle previously approved these changes Oct 6, 2021
View reviewed changes
@ijjk
update tests
6aeccbf
@ijjk
Merge remote-tracking branch 'upstream/canary' into fix/trace-conditi…
a342bed 
…onal-alias

# Conflicts:
#	packages/next/build/webpack-config.ts
#	test/integration/production/test/index.test.js
@ijjk ijjk requested a review from styfle October 6, 2021 15:45
@ijjk
Copy link
Member Author

ijjk commented Oct 6, 2021

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary ijjk/next.js fix/trace-conditional-alias Change
buildDuration 11s 11.2s ⚠️ +211ms
buildDurationCached 2.7s 2.6s -115ms
nodeModulesSize 194 MB 194 MB ⚠️ +117 B
Page Load Tests Overall increase ✓
vercel/next.js canary ijjk/next.js fix/trace-conditional-alias Change
/ failed reqs 0 0 ✓
/ total time (seconds) 2.603 2.478 -0.12
/ avg req/sec 960.44 1008.9 +48.46
/error-in-render failed reqs 0 0 ✓
/error-in-render total time (seconds) 1.255 1.164 -0.09
/error-in-render avg req/sec 1992.35 2147.45 +155.1
Client Bundles (main, webpack, commons)
vercel/next.js canary ijjk/next.js fix/trace-conditional-alias Change
779.HASH.js gzip 179 B 179 B ✓
framework-HASH.js gzip 42.2 kB 42.2 kB ✓
main-HASH.js gzip 26.9 kB 26.9 kB ✓
webpack-HASH.js gzip 1.45 kB 1.45 kB ✓
Overall change 70.8 kB 70.8 kB ✓
Legacy Client Bundles (polyfills)
vercel/next.js canary ijjk/next.js fix/trace-conditional-alias Change
polyfills-a4..dd70.js gzip 31 kB 31 kB ✓
Overall change 31 kB 31 kB ✓
Client Pages
vercel/next.js canary ijjk/next.js fix/trace-conditional-alias Change
_app-HASH.js gzip 977 B 977 B ✓
_error-HASH.js gzip 194 B 194 B ✓
amp-HASH.js gzip 311 B 311 B ✓
css-HASH.js gzip 328 B 328 B ✓
dynamic-HASH.js gzip 2.67 kB 2.67 kB ✓
head-HASH.js gzip 351 B 351 B ✓
hooks-HASH.js gzip 918 B 918 B ✓
image-HASH.js gzip 4.12 kB 4.12 kB ✓
index-HASH.js gzip 260 B 260 B ✓
link-HASH.js gzip 1.66 kB 1.66 kB ✓
routerDirect..HASH.js gzip 320 B 320 B ✓
script-HASH.js gzip 386 B 386 B ✓
withRouter-HASH.js gzip 319 B 319 B ✓
bb14e60e810b..30f.css gzip 125 B 125 B ✓
Overall change 12.9 kB 12.9 kB ✓
Client Build Manifests
vercel/next.js canary ijjk/next.js fix/trace-conditional-alias Change
_buildManifest.js gzip 494 B 494 B ✓
Overall change 494 B 494 B ✓
Rendered Page Sizes
vercel/next.js canary ijjk/next.js fix/trace-conditional-alias Change
index.html gzip 539 B 539 B ✓
link.html gzip 550 B 550 B ✓
withRouter.html gzip 532 B 532 B ✓
Overall change 1.62 kB 1.62 kB ✓
Default Build with SWC (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary ijjk/next.js fix/trace-conditional-alias Change
buildDuration 5.6s 5.7s ⚠️ +55ms
buildDurationCached 2.7s 2.6s -48ms
nodeModulesSize 194 MB 194 MB ⚠️ +117 B
Page Load Tests Overall decrease ⚠️
vercel/next.js canary ijjk/next.js fix/trace-conditional-alias Change
/ failed reqs 0 0 ✓
/ total time (seconds) 2.453 2.6 ⚠️ +0.15
/ avg req/sec 1019.02 961.55 ⚠️ -57.47
/error-in-render failed reqs 0 0 ✓
/error-in-render total time (seconds) 1.175 1.171 0
/error-in-render avg req/sec 2126.93 2135.61 +8.68
Client Bundles (main, webpack, commons)
vercel/next.js canary ijjk/next.js fix/trace-conditional-alias Change
675-HASH.js gzip 13.8 kB 13.8 kB ✓
770.HASH.js gzip 178 B 178 B ✓
framework-HASH.js gzip 50.7 kB 50.7 kB ✓
main-HASH.js gzip 34.9 kB 34.9 kB ✓
webpack-HASH.js gzip 1.64 kB 1.64 kB ✓
Overall change 101 kB 101 kB ✓
Legacy Client Bundles (polyfills)
vercel/next.js canary ijjk/next.js fix/trace-conditional-alias Change
polyfills-a4..dd70.js gzip 31 kB 31 kB ✓
Overall change 31 kB 31 kB ✓
Client Pages
vercel/next.js canary ijjk/next.js fix/trace-conditional-alias Change
_app-HASH.js gzip 1.33 kB 1.33 kB ✓
_error-HASH.js gzip 180 B 180 B ✓
amp-HASH.js gzip 315 B 315 B ✓
css-HASH.js gzip 331 B 331 B ✓
dynamic-HASH.js gzip 2.79 kB 2.79 kB ✓
head-HASH.js gzip 356 B 356 B ✓
hooks-HASH.js gzip 637 B 637 B ✓
image-HASH.js gzip 557 B 557 B ✓
index-HASH.js gzip 262 B 262 B ✓
link-HASH.js gzip 2.2 kB 2.2 kB ✓
routerDirect..HASH.js gzip 326 B 326 B ✓
script-HASH.js gzip 393 B 393 B ✓
withRouter-HASH.js gzip 322 B 322 B ✓
bb14e60e810b..30f.css gzip 125 B 125 B ✓
Overall change 10.1 kB 10.1 kB ✓
Client Build Manifests
vercel/next.js canary ijjk/next.js fix/trace-conditional-alias Change
_buildManifest.js gzip 511 B 511 B ✓
Overall change 511 B 511 B ✓
Rendered Page Sizes
vercel/next.js canary ijjk/next.js fix/trace-conditional-alias Change
index.html gzip 537 B 537 B ✓
link.html gzip 550 B 550 B ✓
withRouter.html gzip 530 B 530 B ✓
Overall change 1.62 kB 1.62 kB ✓
Commit: a342bed

@kodiakhq kodiakhq bot merged commit a20b0d5 into vercel:canary Oct 6, 2021
@ijjk ijjk deleted the fix/trace-conditional-alias branch October 6, 2021 17:19
@vercel vercel locked as resolved and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@styfle styfle styfle approved these changes

@huozhi huozhi Awaiting requested review from huozhi huozhi is a code owner

@padmaia padmaia Awaiting requested review from padmaia

@shuding shuding Awaiting requested review from shuding shuding is a code owner

@timneutkens timneutkens Awaiting requested review from timneutkens timneutkens is a code owner

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.

None yet
2 participants
@ijjk @styfle