-
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
Merged
kodiakhq
merged 9 commits into
vercel:canary
from
baruchadi:feature/friendly-sass-err-msg
May 22, 2022
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
352a131
remove stacktrace, udpate help link & test
baruchadi e319da0
Trim trace stack before clean message&test for it
baruchadi 7086fa0
remove unnecessary test assertion
baruchadi 9515deb
Merge branch 'canary' into feature/friendly-sass-err-msg
baruchadi ecfd011
Merge branch 'canary' into feature/friendly-sass-err-msg
baruchadi 6f0f111
ensure extra error stack is not logged for missing sass
ijjk ccf14e7
Merge branch 'canary' into feature/friendly-sass-err-msg
ijjk b321240
Update link
ijjk b54d2df
Merge branch 'canary' into feature/friendly-sass-err-msg
ijjk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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, nottoContain
, or a better alternative istoMatchInlineSnapshot
so you can drop the extra variable. https://jestjs.io/docs/snapshot-testing#inline-snapshotsThere 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 either
toMatchInlineSnapshot
ortoMatch
would be the right choice here, since thestderr
string contains a lot of machine specific information such as absolute pathsi.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.