-
Notifications
You must be signed in to change notification settings - Fork 142
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
feat: STRF-9749 Update Stencil Cli to use node-sass latest by default #923
Conversation
8eb51c5
to
0256c25
Compare
throw new Error( | ||
`${message}\n`.red + | ||
`\n` + | ||
`---------WARNING---------\n`.red + |
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.
Are you okay with such wordings or do we need to adjust?
cc @bookernath
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.
Looks good 👍 is there some link we can provide to help people diagnose errors? Some node-sass documentation perhaps?
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.
There are bunch of links, that you need to really dig to understand the context:
https://github.com/sass/node-sass/releases/tag/v3.5.1 - the release, that broke and changelogs.
https://github.com/bigcommerce/cornerstone/pull/999/files#diff-7b10e2a8164a3ef7ccb5aa035f393c9715b8945fcc59613d455408c505dd42a5L4 - known example of broken theme case (cornernstone)
sass/libsass#1550 - code examples
sass/sass#779 - code examples
@bookernath Maybe we can include all of them into out changelog and show the link for the changelog?
CHANGELOG.md
Outdated
@@ -1,9 +1,14 @@ | |||
### Draft | |||
|
|||
- feat: strf-9749 Update Stencil Cli to use node-sass latest by default ([x](https://github.com/bigcommerce/stencil-cli/pull/x)) |
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.
Should we include a proper PR number in this?
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.
It's pretty much safe to merge it like that, I would still need to create a release PR and there double check the changelog.
What?
Stencil bundle and stencil push will now validate scss file against latest node-sass version. If it fails, it's still possible to bypass the error by using
--use-old-node-sass-fork
Tickets / Documentation
Screenshots (if appropriate)
cc @bigcommerce/storefront-team