-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Fix: Cleaner error message when importing sass without it being installed in dev #35051
Fix: Cleaner error message when importing sass without it being installed in dev #35051
Conversation
This comment has been minimized.
This comment has been minimized.
packages/next/client/dev/error-overlay/format-webpack-messages.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll get on it! Below is the
Let me know what you think!
|
No, let's focus on the original issue. A better approach to the |
I went with by adding two linebreaks update: remove the unnecessary test assertion for "Require stack" |
Stats from current PRDefault Build (Decrease detected ✓)General Overall increase
|
vercel/next.js canary | baruchadi/next.js feature/friendly-sass-err-msg | Change | |
---|---|---|---|
buildDuration | 18.8s | 19.1s | |
buildDurationCached | 7.4s | 7.2s | -158ms |
nodeModulesSize | 479 MB | 479 MB |
Page Load Tests Overall decrease ⚠️
vercel/next.js canary | baruchadi/next.js feature/friendly-sass-err-msg | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 4.93 | 5.078 | |
/ avg req/sec | 507.09 | 492.34 | |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 2.049 | 2.032 | -0.02 |
/error-in-render avg req/sec | 1220.27 | 1230.17 | +9.9 |
Client Bundles (main, webpack)
vercel/next.js canary | baruchadi/next.js feature/friendly-sass-err-msg | Change | |
---|---|---|---|
925.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42 kB | 42 kB | ✓ |
main-HASH.js gzip | 29 kB | 29 kB | ✓ |
webpack-HASH.js gzip | 1.54 kB | 1.54 kB | ✓ |
Overall change | 72.8 kB | 72.8 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | baruchadi/next.js feature/friendly-sass-err-msg | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | baruchadi/next.js feature/friendly-sass-err-msg | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.36 kB | 1.36 kB | ✓ |
_error-HASH.js gzip | 193 B | 193 B | ✓ |
amp-HASH.js gzip | 308 B | 308 B | ✓ |
css-HASH.js gzip | 327 B | 327 B | ✓ |
dynamic-HASH.js gzip | 2.71 kB | 2.71 kB | ✓ |
head-HASH.js gzip | 359 B | 359 B | ✓ |
hooks-HASH.js gzip | 920 B | 920 B | ✓ |
image-HASH.js gzip | 5.73 kB | 5.73 kB | ✓ |
index-HASH.js gzip | 263 B | 263 B | ✓ |
link-HASH.js gzip | 2.65 kB | 2.65 kB | ✓ |
routerDirect..HASH.js gzip | 320 B | 320 B | ✓ |
script-HASH.js gzip | 391 B | 391 B | ✓ |
withRouter-HASH.js gzip | 318 B | 318 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 16 kB | 16 kB | ✓ |
Client Build Manifests
vercel/next.js canary | baruchadi/next.js feature/friendly-sass-err-msg | Change | |
---|---|---|---|
_buildManifest.js gzip | 458 B | 458 B | ✓ |
Overall change | 458 B | 458 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | baruchadi/next.js feature/friendly-sass-err-msg | Change | |
---|---|---|---|
index.html gzip | 532 B | 532 B | ✓ |
link.html gzip | 546 B | 546 B | ✓ |
withRouter.html gzip | 527 B | 527 B | ✓ |
Overall change | 1.6 kB | 1.6 kB | ✓ |
Default Build with SWC (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary | baruchadi/next.js feature/friendly-sass-err-msg | Change | |
---|---|---|---|
buildDuration | 21.6s | 21.6s | -28ms |
buildDurationCached | 7.3s | 7.3s | |
nodeModulesSize | 479 MB | 479 MB |
Page Load Tests Overall decrease ⚠️
vercel/next.js canary | baruchadi/next.js feature/friendly-sass-err-msg | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 5.027 | 4.877 | -0.15 |
/ avg req/sec | 497.28 | 512.65 | +15.37 |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.972 | 2.049 | |
/error-in-render avg req/sec | 1267.45 | 1220.24 |
Client Bundles (main, webpack)
vercel/next.js canary | baruchadi/next.js feature/friendly-sass-err-msg | Change | |
---|---|---|---|
925.HASH.js gzip | 178 B | 178 B | ✓ |
framework-HASH.js gzip | 42.7 kB | 42.7 kB | ✓ |
main-HASH.js gzip | 29.5 kB | 29.5 kB | ✓ |
webpack-HASH.js gzip | 1.54 kB | 1.54 kB | ✓ |
Overall change | 73.9 kB | 73.9 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | baruchadi/next.js feature/friendly-sass-err-msg | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | baruchadi/next.js feature/friendly-sass-err-msg | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.35 kB | 1.35 kB | ✓ |
_error-HASH.js gzip | 179 B | 179 B | ✓ |
amp-HASH.js gzip | 311 B | 311 B | ✓ |
css-HASH.js gzip | 324 B | 324 B | ✓ |
dynamic-HASH.js gzip | 2.89 kB | 2.89 kB | ✓ |
head-HASH.js gzip | 357 B | 357 B | ✓ |
hooks-HASH.js gzip | 920 B | 920 B | ✓ |
image-HASH.js gzip | 5.82 kB | 5.82 kB | ✓ |
index-HASH.js gzip | 261 B | 261 B | ✓ |
link-HASH.js gzip | 2.78 kB | 2.78 kB | ✓ |
routerDirect..HASH.js gzip | 322 B | 322 B | ✓ |
script-HASH.js gzip | 392 B | 392 B | ✓ |
withRouter-HASH.js gzip | 317 B | 317 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 16.3 kB | 16.3 kB | ✓ |
Client Build Manifests
vercel/next.js canary | baruchadi/next.js feature/friendly-sass-err-msg | Change | |
---|---|---|---|
_buildManifest.js gzip | 457 B | 457 B | ✓ |
Overall change | 457 B | 457 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | baruchadi/next.js feature/friendly-sass-err-msg | Change | |
---|---|---|---|
index.html gzip | 533 B | 533 B | ✓ |
link.html gzip | 547 B | 547 B | ✓ |
withRouter.html gzip | 529 B | 529 B | ✓ |
Overall change | 1.61 kB | 1.61 kB | ✓ |
expect(stderr).toContain( | ||
'Learn more: https://nextjs.org/docs/messages/install-sass' | ||
) | ||
expect(stderr).toContain(cleanScssErrMsg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be using toMatch
for an exact match, not toContain
, or a better alternative is toMatchInlineSnapshot
so you can drop the extra variable. https://jestjs.io/docs/snapshot-testing#inline-snapshots
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that eithertoMatchInlineSnapshot
or toMatch
would be the right choice here, since the stderr
string contains a lot of machine specific information such as absolute paths
i.e.
at new Input (/Users/baruchadi/projects/next/next.js/packages/next/node_modules/postcss/lib/input.js:24:13)
and given that the string is about 60 lines long and has more than than 6500 characters (see example value here: https://gist.github.com/baruchadi/81be9d5b4d1f45d6f1f22b2195777a9a), I believe that .toContain
would be our best bet at the moment.
We can do .toMatch
with some regex, but that would require us to make a lot of assumptions and the result would be very similar to .toContain
.
Note that in the code, i'm checking for two new lines before and after our clean message. this should guarantee that the stack was removed from above and below our message.
see the diff between our error message and the original unedited one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't the point of this PR to only have https://gist.github.com/baruchadi/81be9d5b4d1f45d6f1f22b2195777a9a#file-stderr-output-L4-L8 ? That should always be the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make the assertion exact since we are trying to verify that there is no leftover in the output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
This PR removes the not-very-helpful stack trace when sass is being used but the npm package is not installed. Fixes #13975
Bug
fixes #13975
contributing.md
Feature
fixes #number
contributing.md
Documentation / Examples
yarn lint