-
-
Notifications
You must be signed in to change notification settings - Fork 64
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: adopt @use syntax to support new Sass module system #111
Conversation
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 great. @lucpotage apologies for the slow reply. This change seems useful.
- What additional work do you foresee?
- We need to pass CI.
Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lucpotage)
CHANGELOG.md, line 5 at r1 (raw file):
## [3.0.0] - 2020-01-10 * Supports the new Sass module system released with Dart Sass 1.23.0. The use of @use is recommended instead of @import.
We need migration instructions if this is not backward compatible.
README.md, line 7 at r1 (raw file):
[](https://www.npmjs.com/package/sass-resources-loader) This loader will `@use` your SASS resources into every `required` SASS module. So you can use your shared variables, mixins and functions across all SASS styles without manually loading them in each file. Made to work with CSS Modules!
Need a comment in the README if the new version requires a newer version of sass.
test/scss/shared/_index.scss, line 1 at r1 (raw file):
@forward "shared/variables";
it would be great to provide some explanation in the README of when to use @forward vs @use.
I'm guessing this comment might work:
// Note the use of @forward vs. @use.
// @forward will make the members inside of "shared/variables" available to any files that @use this file.
// If @use were used, then nothing would be passed along to any imports of this file.
This is a great reference: https://css-tricks.com/introducing-sass-modules/#article-header-id-0
Maybe we should put in the README?
@lucpotage Any chance that you can take a look at my comments? |
@justin808 I checked how the webpack sass-loader works and it seems they resolve the implementation based on the chosen dependency. What about doing the same? The sass docs says:
What is not clear to me is whether we should force the use of @use in case you have the newest Do you think adding a loader option {
loader: 'sass-resources-loader',
options: {
rule: '@use', // or @import
},
} Getting feedback from @nex3 would help. |
@lucpotage I like your last idea. Can you take a try at implementing it? |
@lucpotage did you see my previous comment? |
Yes and I would like to try at implementing it but I definitely don't have the time right now. 😞 |
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.
@lucpotage Thanks for this PR, I am looking forward to being able to use this!
Apart from the rule
option that will take some more time to implement, I found a few small nits.
[Unreleased]: https://github.com/shakacode/sass-resources-loader/compare/v2.0.0...master | ||
[2.0.0]: https://github.com/shakacode/sass-resources-loader/compare/v2.0.0...v3.0.0 |
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.
[Unreleased]: https://github.com/shakacode/sass-resources-loader/compare/v2.0.0...master | |
[2.0.0]: https://github.com/shakacode/sass-resources-loader/compare/v2.0.0...v3.0.0 | |
[Unreleased]: https://github.com/shakacode/sass-resources-loader/compare/v3.0.0...master | |
[3.0.0]: https://github.com/shakacode/sass-resources-loader/compare/v2.0.0...v3.0.0 |
We've got some merge conflicts. And some CI issues. Let me know when this is ready. @lucpotage and @FloEdelmann, thank you for your efforts! Feel free to email me [email protected]. I've got a slack channel for this as well. You can find the link for Slack here: https://www.shakacode.com/open-source-projects. |
Co-authored-by: Flo Edelmann <[email protected]>
Co-authored-by: Flo Edelmann <[email protected]>
See #117. |
This definitely requires additional work but I hope you can find some time to test this PR and improve it. Thank you in advance.
I didn't bumped the npm version.
This change is