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

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

Merged
merged 9 commits into from
May 22, 2022
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
352a131
remove stacktrace, udpate help link & test
baruchadi Mar 4, 2022
e319da0
Trim trace stack before clean message&test for it
baruchadi Mar 7, 2022
7086fa0
remove unnecessary test assertion
baruchadi Mar 7, 2022
9515deb
Merge branch 'canary' into feature/friendly-sass-err-msg
baruchadi Mar 7, 2022
ecfd011
Merge branch 'canary' into feature/friendly-sass-err-msg
baruchadi Mar 15, 2022
6f0f111
ensure extra error stack is not logged for missing sass
ijjk May 22, 2022
ccf14e7
Merge branch 'canary' into feature/friendly-sass-err-msg
ijjk May 22, 2022
b321240
Update link
ijjk May 22, 2022
b54d2df
Merge branch 'canary' into feature/friendly-sass-err-msg
ijjk May 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions packages/next/client/dev/error-overlay/format-webpack-messages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,14 +111,18 @@ function formatMessage(message, verbose) {
}

// Add helpful message for users trying to use Sass for the first time
if (lines[1] && lines[1].match(/Cannot find module.+node-sass/)) {
if (lines[1] && lines[1].match(/Cannot find module.+sass/)) {
// ./file.module.scss (<<loader info>>) => ./file.module.scss
lines[0] = lines[0].replace(/(.+) \(.+?(?=\?\?).+?\)/, '$1')
const firstLine = lines[0].split('!')
lines[0] = firstLine[firstLine.length - 1]

lines[1] =
"To use Next.js' built-in Sass support, you first need to install `sass`.\n"
lines[1] += 'Run `npm i sass` or `yarn add sass` inside your workspace.\n'
lines[1] += '\nLearn more: https://nextjs.org/docs/messages/install-sass'
lines[1] += '\nLearn more: https://err.sh/next.js/install-sass'

// dispose of unhelpful stack trace
lines = lines.slice(0, 2)
}

if (!verbose) {
Expand Down
19 changes: 10 additions & 9 deletions test/integration/scss/test/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,18 @@ describe('SCSS Support', () => {
env: { NODE_OPTIONS: shellQuote([`--require`, mockFile]) },
stderr: true,
})
let cleanScssErrMsg =
'\n\n' +
'./styles/global.scss\n' +
"To use Next.js' built-in Sass support, you first need to install `sass`.\n" +
'Run `npm i sass` or `yarn add sass` inside your workspace.\n' +
'\n' +
'Learn more: https://err.sh/next.js/install-sass\n' +
'\n'

expect(code).toBe(1)
expect(stderr).toContain('Failed to compile.')
expect(stderr).toContain(
"To use Next.js' built-in Sass support, you first need to install `sass`."
)
expect(stderr).toContain(
'Run `npm i sass` or `yarn add sass` inside your workspace.'
)
expect(stderr).toContain(
'Learn more: https://nextjs.org/docs/messages/install-sass'
)
expect(stderr).toContain(cleanScssErrMsg)
Copy link
Member

@balazsorban44 balazsorban44 Mar 7, 2022 •
edited
Loading

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

Copy link
Contributor 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 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
image

Copy link
Member

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.

})
})

Expand Down