Fix: Cleaner error message when importing sass without it being installed in dev #35051

baruchadi · 2022-03-04T16:57:00Z

This PR removes the not-very-helpful stack trace when sass is being used but the npm package is not installed. Fixes #13975

Fix behavior to show the modified error message if either node-sass OR sass is missing
dispose of stack trace if the condition above passes
update the error link to err.sh equivalent
update the relevant test to verify the stack trace is omitted and to account for the new link

Bug

Related issues linked using fixes #13975
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

packages/next/client/dev/error-overlay/format-webpack-messages.js

test/integration/scss/test/index.test.js

balazsorban44

I'm seeing the following on a global Sass import:

Similar to the CSS module, let's make sure to clean up the stack trace before the custom error message.

baruchadi · 2022-03-07T15:26:58Z

@balazsorban44

Similar to the CSS module, let's make sure to clean up the stack trace before the custom error message.

Sounds good, I'll get on it!
To get some more clarity before I make this modification:

Below is the stderr string on which the tests are performed, it seems like there are a lot of subsequent errors besides the one I'm working on which have the ruleset substring. That would cause the new test assertion you recommended to fail, regardless of if I use the approach you mentioned above to clean up the trace before the custom error message.

expect(stderr).not.toContain('ruleSet')

Should these other error messages be cleaned up as well, as part the scope of this issue/PR?
If not, and the rest of the errors are behaving as expected - I'll have to apply these tests to a part of the stderr string for the tests to pass correctly while keeping the new assertion.
- the new string on which I'll test will be the first 9 lines, or in other words until a second ./styles/... is found.

Let me know what you think!

warn  - The Next.js plugin was not detected in your ESLint configuration. See https://nextjs.org/docs/basic- features/eslint#migrating-existing-config
Failed to compile.

./styles/global.scss
To use Next.js' built-in Sass support, you first need to install `sass`.
Run `npm i sass` or `yarn add sass` inside your workspace.

Learn more: https://err.sh/next.js/install-sass

./styles/global.scss.webpack[javascript/auto]!=!../../../../packages/next/dist/build/webpack/loaders/css-loader/src/index.js??ruleSet[1].rules[3].oneOf[9].use[1]!../../../../packages/next/dist/build/webpack/loaders/postcss-loader/src/index.js??ruleSet[1].rules[3].oneOf[9].use[2]!../../../../packages/next/dist/build/webpack/loaders/resolve-url-loader/index.js??ruleSet[1].rules[3].oneOf[9].use[3]!../../../../packages/next/dist/compiled/sass-loader/cjs.js??ruleSet[1].rules[3].oneOf[9].use[4]!./styles/global.scss
Error: resolve-url-loader: CSS error
  PostCSS received undefined instead of CSS string
  at new Input (/Users/baruchadi/projects/next/next.js/packages/next/node_modules/postcss/lib/input.js:24:13)
    at encodeError (/Users/baruchadi/projects/next/next.js/packages/next/dist/build/webpack/loaders/resolve-url-loader/index.js:52:16)
    at onFailure (/Users/baruchadi/projects/next/next.js/packages/next/dist/build/webpack/loaders/resolve-url-loader/index.js:38:18)

Import trace for requested module:
./styles/global.scss
./pages/_app.js

./styles/global.scss
Error: resolve-url-loader: CSS error
  PostCSS received undefined instead of CSS string
  at new Input (/Users/baruchadi/projects/next/next.js/packages/next/node_modules/postcss/lib/input.js:24:13)
    at encodeError (/Users/baruchadi/projects/next/next.js/packages/next/dist/build/webpack/loaders/resolve-url-loader/index.js:52:16)
    at onFailure (/Users/baruchadi/projects/next/next.js/packages/next/dist/build/webpack/loaders/resolve-url-loader/index.js:38:18)
    at tryRunOrWebpackError (/Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/webpack/bundle5.js:41704:9)
    at __webpack_require_module__ (/Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/webpack/bundle5.js:27321:12)
    at __nested_webpack_require_152432__ (/Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/webpack/bundle5.js:27278:18)
    at /Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/webpack/bundle5.js:27349:20
    at symbolIterator (/Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/neo-async/async.js:1:14443)
    at done (/Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/neo-async/async.js:1:14823)
    at Hook.eval [as callAsync] (eval at create (/Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/webpack/bundle5.js:140346:10), <anonymous>:13:1)
    at Hook.CALL_ASYNC_DELEGATE [as _callAsync] (/Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/webpack/bundle5.js:140148:14)
    at /Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/webpack/bundle5.js:27256:43
    at symbolIterator (/Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/neo-async/async.js:1:14401)
-- inner error --
Error: resolve-url-loader: CSS error
  PostCSS received undefined instead of CSS string
  at new Input (/Users/baruchadi/projects/next/next.js/packages/next/node_modules/postcss/lib/input.js:24:13)
    at encodeError (/Users/baruchadi/projects/next/next.js/packages/next/dist/build/webpack/loaders/resolve-url-loader/index.js:52:16)
    at onFailure (/Users/baruchadi/projects/next/next.js/packages/next/dist/build/webpack/loaders/resolve-url-loader/index.js:38:18)
    at Object.<anonymous> (/Users/baruchadi/projects/next/next.js/packages/next/dist/build/webpack/loaders/css-loader/src/index.js??ruleSet[1].rules[3].oneOf[9].use[1]!/Users/baruchadi/projects/next/next.js/packages/next/dist/build/webpack/loaders/postcss-loader/src/index.js??ruleSet[1].rules[3].oneOf[9].use[2]!/Users/baruchadi/projects/next/next.js/packages/next/dist/build/webpack/loaders/resolve-url-loader/index.js??ruleSet[1].rules[3].oneOf[9].use[3]!/Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/sass-loader/cjs.js??ruleSet[1].rules[3].oneOf[9].use[4]!/Users/baruchadi/projects/next/next.js/test/integration/scss-fixtures/webpack-error/styles/global.scss:1:7)
    at /Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/webpack/bundle5.js:90227:11
    at Hook.eval [as call] (eval at create (/Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/webpack/bundle5.js:140332:10), <anonymous>:5:1)
    at Hook.CALL_DELEGATE [as _call] (/Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/webpack/bundle5.js:140144:14)
    at /Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/webpack/bundle5.js:27323:39
    at tryRunOrWebpackError (/Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/webpack/bundle5.js:41699:7)
    at __webpack_require_module__ (/Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/webpack/bundle5.js:27321:12)
    at __nested_webpack_require_152432__ (/Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/webpack/bundle5.js:27278:18)
    at /Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/webpack/bundle5.js:27349:20
    at symbolIterator (/Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/neo-async/async.js:1:14443)

Generated code for /Users/baruchadi/projects/next/next.js/packages/next/dist/build/webpack/loaders/css-loader/src/index.js??ruleSet[1].rules[3].oneOf[9].use[1]!/Users/baruchadi/projects/next/next.js/packages/next/dist/build/webpack/loaders/postcss-loader/src/index.js??ruleSet[1].rules[3].oneOf[9].use[2]!/Users/baruchadi/projects/next/next.js/packages/next/dist/build/webpack/loaders/resolve-url-loader/index.js??ruleSet[1].rules[3].oneOf[9].use[3]!/Users/baruchadi/projects/next/next.js/packages/next/dist/compiled/sass-loader/cjs.js??ruleSet[1].rules[3].oneOf[9].use[4]!/Users/baruchadi/projects/next/next.js/test/integration/scss-fixtures/webpack-error/styles/global.scss

Import trace for requested module:
./pages/_app.js


> Build failed because of webpack errors`

balazsorban44 · 2022-03-07T15:51:29Z

Should these other error messages be cleaned up as well, as part of the scope of this issue/PR?

No, let's focus on the original issue.

A better approach to the expect assertion would be to do expect(stderr).toBe("...") with an exact match of the result. 👍

test/integration/scss/test/index.test.js

baruchadi · 2022-03-07T16:19:57Z

I went with
expect(stderr).toContain(cleanScssErrMsg)
instead of
expect(stderr).toBe("...")
Because the stderr is just so large (check my previous comment above) and I wanted to avoid snapshotting.

by adding two linebreaks \n\n both before and after the expected "clean message", we can assert that there is no trace prepended or appended to our "clean message"

update: remove the unnecessary test assertion for "Require stack"

ijjk · 2022-03-07T16:57:22Z

Stats from current PR

Default Build (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	baruchadi/next.js feature/friendly-sass-err-msg	Change
buildDuration	18.8s	19.1s	⚠️ +327ms
buildDurationCached	7.4s	7.2s	-158ms
nodeModulesSize	479 MB	479 MB	⚠️ +808 B

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	⚠️ +0.15
/ avg req/sec	507.09	492.34	⚠️ -14.75
/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	⚠️ +33ms
nodeModulesSize	479 MB	479 MB	⚠️ +808 B

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	⚠️ +0.08
/error-in-render avg req/sec	1267.45	1220.24	⚠️ -47.21

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	✓

Commit: b54d2df

balazsorban44 · 2022-03-07T17:28:58Z

test/integration/scss/test/index.test.js

-      expect(stderr).toContain(
-        'Learn more: https://nextjs.org/docs/messages/install-sass'
-      )
+      expect(stderr).toContain(cleanScssErrMsg)


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

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

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.

balazsorban44

Let's make the assertion exact since we are trying to verify that there is no leftover in the output.

updated

ijjk

Thanks for the PR!

remove stacktrace, udpate help link & test

352a131

baruchadi requested review from timneutkens, ijjk, shuding and huozhi as code owners March 4, 2022 16:57

ijjk added the type: next label Mar 4, 2022

baruchadi changed the title ~~Fix error message when importing sass without it being installed in dev~~ Fix: Cleaner error message when importing sass without it being installed in dev Mar 4, 2022

baruchadi mentioned this pull request Mar 6, 2022

Bad error message when importing sass without it being installed in dev #13975

Closed

This comment has been minimized.

Sign in to view

balazsorban44 reviewed Mar 7, 2022

View reviewed changes

packages/next/client/dev/error-overlay/format-webpack-messages.js Outdated Show resolved Hide resolved

balazsorban44 reviewed Mar 7, 2022

View reviewed changes

test/integration/scss/test/index.test.js Outdated Show resolved Hide resolved

balazsorban44 requested changes Mar 7, 2022

View reviewed changes

Trim trace stack before clean message&test for it

e319da0

balazsorban44 reviewed Mar 7, 2022

View reviewed changes

test/integration/scss/test/index.test.js Outdated Show resolved Hide resolved

remove unnecessary test assertion

7086fa0

Merge branch 'canary' into feature/friendly-sass-err-msg

9515deb

baruchadi requested a review from balazsorban44 March 7, 2022 16:36

balazsorban44 reviewed Mar 7, 2022

View reviewed changes

balazsorban44 previously requested changes Mar 7, 2022

View reviewed changes

baruchadi and others added 4 commits March 15, 2022 13:40

Merge branch 'canary' into feature/friendly-sass-err-msg

ecfd011

ensure extra error stack is not logged for missing sass

6f0f111

Merge branch 'canary' into feature/friendly-sass-err-msg

ccf14e7

Update link

b321240

baruchadi requested a review from padmaia as a code owner May 22, 2022 06:19

ijjk approved these changes May 22, 2022

View reviewed changes

Merge branch 'canary' into feature/friendly-sass-err-msg

b54d2df

kodiakhq bot merged commit e2fde26 into vercel:canary May 22, 2022

github-actions bot locked as resolved and limited conversation to collaborators Jun 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

baruchadi commented Mar 4, 2022

This comment has been minimized.

balazsorban44 left a comment •

edited

Loading

baruchadi commented Mar 7, 2022

balazsorban44 commented Mar 7, 2022

baruchadi commented Mar 7, 2022 •

edited

Loading

ijjk commented Mar 7, 2022 •

edited

Loading

balazsorban44 Mar 7, 2022 •

edited

Loading

baruchadi Mar 7, 2022

balazsorban44 Mar 8, 2022

balazsorban44 left a comment

ijjk left a comment

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

baruchadi commented Mar 4, 2022

Bug

Feature

Documentation / Examples

This comment has been minimized.

balazsorban44 left a comment • edited Loading

Choose a reason for hiding this comment

baruchadi commented Mar 7, 2022

balazsorban44 commented Mar 7, 2022

baruchadi commented Mar 7, 2022 • edited Loading

ijjk commented Mar 7, 2022 • edited Loading

Stats from current PR

balazsorban44 Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

baruchadi Mar 7, 2022

Choose a reason for hiding this comment

balazsorban44 Mar 8, 2022

Choose a reason for hiding this comment

balazsorban44 left a comment

Choose a reason for hiding this comment

ijjk left a comment

Choose a reason for hiding this comment

balazsorban44 left a comment •

edited

Loading

baruchadi commented Mar 7, 2022 •

edited

Loading

ijjk commented Mar 7, 2022 •

edited

Loading

balazsorban44 Mar 7, 2022 •

edited

Loading