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

Remove unpurpose test #31898

Merged
merged 3 commits into from
Nov 29, 2021
Merged

Remove unpurpose test #31898

merged 3 commits into from
Nov 29, 2021

Conversation

huozhi
Copy link
Member

@huozhi huozhi commented Nov 28, 2021

The test from #31784 can pass with or without the source code change in that PR.
I cannot repro it. The writable is true on build in the provided case

@ijjk ijjk added the created-by: Next.js team PRs by the Next.js team. label Nov 28, 2021
@huozhi huozhi requested a review from styfle November 28, 2021 23:40
@ijjk

This comment has been minimized.

@awareness481
Copy link
Contributor

My intention was for it to act for a regression test but it's highly unlikely the bug gets reintroduced in the same the manner (imo).

@huozhi
Copy link
Member Author

huozhi commented Nov 29, 2021

@awareness481 Thanks for the explanation, I think would be great if we can find any case that repro that issue and convert it to a test case.

@awareness481
Copy link
Contributor

awareness481 commented Nov 29, 2021

@awareness481 Thanks for the explanation, I think would be great if we can find any case that repro that issue and convert it to a test case.

Oh my bad, I didn't see that you said that you can't repro without the merged PR. I only tried a couple Next.js versions just now and I couldn't reproduce, I'll try to investigate more when I have more free time.

In the meantime, FWIW I agree the the test should be removed (and possibly the original PR should be reverted?).

Edit: Right now I can only reproduce with next-boost as the original comment suggests

Tried 12.0.4 & 12.0.5-canary.9 on this repo and the issue was reproduced. I distinctly remember being able to reproduce without next-boost but it's been three months since the PR and I could have also been mistaken.

@ijjk

This comment has been minimized.

@ijjk
Copy link
Member

ijjk commented Nov 29, 2021

Stats from current PR

Default Build (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary huozhi/next.js rm-test Change
buildDuration 17.5s 17.3s -262ms
buildDurationCached 3.3s 3.3s ⚠️ +14ms
nodeModulesSize 346 MB 346 MB ⚠️ +2 B
Page Load Tests Overall decrease ⚠️
vercel/next.js canary huozhi/next.js rm-test Change
/ failed reqs 0 0
/ total time (seconds) 2.879 2.781 -0.1
/ avg req/sec 868.24 898.85 +30.61
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.322 1.344 ⚠️ +0.02
/error-in-render avg req/sec 1890.79 1860.24 ⚠️ -30.55
Client Bundles (main, webpack, commons)
vercel/next.js canary huozhi/next.js rm-test Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42.2 kB 42.2 kB
main-HASH.js gzip 28.4 kB 28.4 kB
webpack-HASH.js gzip 1.45 kB 1.45 kB
Overall change 72.2 kB 72.2 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary huozhi/next.js rm-test Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary huozhi/next.js rm-test 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.38 kB 2.38 kB
head-HASH.js gzip 350 B 350 B
hooks-HASH.js gzip 635 B 635 B
image-HASH.js gzip 4.45 kB 4.45 kB
index-HASH.js gzip 263 B 263 B
link-HASH.js gzip 1.87 kB 1.87 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 13.3 kB 13.3 kB
Client Build Manifests
vercel/next.js canary huozhi/next.js rm-test Change
_buildManifest.js gzip 460 B 460 B
Overall change 460 B 460 B
Rendered Page Sizes
vercel/next.js canary huozhi/next.js rm-test 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

Default Build with SWC (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary huozhi/next.js rm-test Change
buildDuration 19.2s 19.6s ⚠️ +404ms
buildDurationCached 3.3s 3.5s ⚠️ +198ms
nodeModulesSize 346 MB 346 MB ⚠️ +2 B
Page Load Tests Overall decrease ⚠️
vercel/next.js canary huozhi/next.js rm-test Change
/ failed reqs 0 0
/ total time (seconds) 2.827 2.936 ⚠️ +0.11
/ avg req/sec 884.33 851.62 ⚠️ -32.71
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.283 1.322 ⚠️ +0.04
/error-in-render avg req/sec 1947.99 1890.65 ⚠️ -57.34
Client Bundles (main, webpack, commons)
vercel/next.js canary huozhi/next.js rm-test Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42.3 kB 42.3 kB
main-HASH.js gzip 28.6 kB 28.6 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 72.5 kB 72.5 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary huozhi/next.js rm-test Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary huozhi/next.js rm-test 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.37 kB 2.37 kB
head-HASH.js gzip 342 B 342 B
hooks-HASH.js gzip 622 B 622 B
image-HASH.js gzip 4.47 kB 4.47 kB
index-HASH.js gzip 256 B 256 B
link-HASH.js gzip 1.91 kB 1.91 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 13.2 kB 13.2 kB
Client Build Manifests
vercel/next.js canary huozhi/next.js rm-test Change
_buildManifest.js gzip 459 B 459 B
Overall change 459 B 459 B
Rendered Page Sizes
vercel/next.js canary huozhi/next.js rm-test Change
index.html gzip 532 B 532 B
link.html gzip 544 B 544 B
withRouter.html gzip 526 B 526 B
Overall change 1.6 kB 1.6 kB
Commit: 11d3662

@kodiakhq kodiakhq bot merged commit 6920c02 into vercel:canary Nov 29, 2021
@huozhi huozhi deleted the rm-test branch November 29, 2021 15:06
@huozhi
Copy link
Member Author

huozhi commented Nov 29, 2021

@awareness481 I filed another PR with explaination in PR description here in #31929

kodiakhq bot pushed a commit that referenced this pull request Nov 30, 2021
x-ref: #28498, #31784

I can repro the issue #24783 with `next-boost` 0.10.1 and it was fixed in 0.10.2 (ref: next-boost/next-boost@eba6d10). The actual case is missing setting node env to `"production"`.

React freeze element props and element itself in dev mode (ref: https://github.com/facebook/react/blob/a724a3b578dce77d427bef313102a4d0e978d9b4/packages/react/src/ReactElement.js#L194-L196)

Then next.js will reassign props with react development bundle while next-boost is enabled. Those operations are only happened in non-dev mode so it's good to remove now.

This PR + #31898 == revert #31784

cc @styfle @awareness481
@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.
Labels
created-by: Next.js team PRs by the Next.js team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants