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.

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

Closed
wants to merge 3 commits into from

Conversation

lucpotage
Copy link

@lucpotage lucpotage commented Jan 10, 2020

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 Reviewable

Copy link
Member

@justin808 justin808 left a 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.

  1. What additional work do you foresee?
  2. 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):

[![license](https://img.shields.io/npm/l/sass-resources-loader.svg?style=flat-square)](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?

@justin808
Copy link
Member

@lucpotage Any chance that you can take a look at my comments?

@lucpotage
Copy link
Author

lucpotage commented Feb 5, 2020

@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:

Sass will gradually phase it out over the next few years, and eventually remove it from the language entirely.

What is not clear to me is whether we should force the use of @use in case you have the newest sass dependency whereas @import is clearly not yet deprecated.

Do you think adding a loader option rule would make sense? With @import or @use possible values and @use by default to follow the recommendation.

{
  loader: 'sass-resources-loader',
  options: {
    rule: '@use', // or @import
  },
}

Getting feedback from @nex3 would help.

@justin808
Copy link
Member

@lucpotage I like your last idea. Can you take a try at implementing it?

@justin808
Copy link
Member

@lucpotage did you see my previous comment?

@lucpotage
Copy link
Author

lucpotage commented Feb 18, 2020

Yes and I would like to try at implementing it but I definitely don't have the time right now. 😞

Copy link

@FloEdelmann FloEdelmann left a 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.

README.md Outdated Show resolved Hide resolved
Comment on lines 29 to +30
[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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[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

package.json Outdated Show resolved Hide resolved
@justin808
Copy link
Member

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 justin@shakacode.com. 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.

lucpotage and others added 2 commits July 23, 2020 20:34
Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
@justin808
Copy link
Member

See #117.

@justin808 justin808 closed this Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants