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

test: sass modules "@use" #770

Merged
merged 1 commit into from Oct 7, 2019
Merged

test: sass modules "@use" #770

merged 1 commit into from Oct 7, 2019

Conversation

donnysim
Copy link
Contributor

@donnysim donnysim commented Oct 5, 2019

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Sass launched modules feature not too long ago and its path resolution works the same way as @import does, no changes are required to support it. There are slight differences for @use vs @import when testing:

  • All @use are resolved only once no matter how many times the same file is included;
  • Requires quotes in scss and sass;
  • Imports like ~bootstrap require alias - @use "~bootstrap" as bootstrap because ~bootstrap is invalid name in sass;
  • Does not support @use url - that's @import responsibility;
  • Does not support multiple @use in one line separated by comma - each @use must be on new line;
  • Does not support css nesting like .class { @import "path" };

At the moment only Dart Sass 1.23.0 and up support @use feature so bumped sass dependency.

Breaking Changes

None.

Additional Info

http://sass.logdown.com/posts/7858341-the-module-system-is-launched

@jsf-clabot
Copy link

jsf-clabot commented Oct 5, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Oct 5, 2019

Codecov Report

Merging #770 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #770   +/-   ##
=======================================
  Coverage   97.75%   97.75%           
=======================================
  Files          10       10           
  Lines         178      178           
  Branches       63       63           
=======================================
  Hits          174      174           
  Misses          4        4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa9b53b...d9eeb1b. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Awesome job! You are wonderful!

@alexander-akait
Copy link
Member

Big thanks!

@alexander-akait alexander-akait merged commit 185ba80 into webpack-contrib:master Oct 7, 2019
@donnysim donnysim deleted the test-sass-modules branch October 8, 2019 08:45
@egze
Copy link

egze commented Oct 21, 2019

Sorry, does it mean I can use @use if I install the latest sass-loader?

@donnysim
Copy link
Contributor Author

@egze only thing you need is to install sass v1.23 or later to use it and make sure you don't have node-sass dependency (it doesn't support @use yet, or keep both but configure sass-loader to use sass). No changes are needed for sass-loader to support it as it uses the same path resolution implementation as @import.

@mitinarseny
Copy link

When using dart sass, @use '~some_thing/another' do not resolve ~. Any help?

@donnysim
Copy link
Contributor Author

donnysim commented Nov 7, 2019

@mitinarseny I tested it and it does resolve it - things like @use "~bootstrap/scss/function" all work without any problems. If your import is something like @use "~bootstrap" then you must add as some-name as in @use "~bootstrap" as bootstrap etc. because it cannot create a module variable named ~bootstrap.
If you have some other problem, would be nice to have more info.

@mitinarseny
Copy link

mitinarseny commented Nov 7, 2019

@donnysim Sure:

ERROR in ./src/sass/main.scss (./node_modules/css-loader/dist/cjs.js!./node_modules/postcss-loader/src??ref--5-2!./node_modules/sass-loader/dist/cjs.js??ref--5-3!./src/sass/main.scss)
Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
SassError: Can't find stylesheet to import.
  ╷
1 │ @use '~katex/dist/katex.css';
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  src/sass/base/_typography.scss 1:1  @use
  src/sass/base/_index.scss 2:1       @use
  /Users/mitinarseny/dev/hse/statistics/hw_new/src/sass/main.scss 4:1                           root stylesheet
 @ ./src/sass/main.scss 1:14-190 20:4-31:5 23:25-201
 @ ./src/main.js
# make sure that node_modules/katex/dist/katex.css exists
$ ls node_modules/katex/dist
README.md     contrib       fonts         katex.css     katex.js      katex.min.css katex.min.js  katex.mjs
// in main.js

import './sass/main.scss';
$ tree sass
sass
├── abstracts
│   ├── _index.scss
│   └── _variables.scss
├── base
│   ├── _index.scss
│   ├── _reset.scss
│   └── _typography.scss
├── layout
│   ├── _grid.scss
│   └── _index.scss
└── main.scss
// sass/main.scss

@charset 'utf-8';

@use 'abstracts';
@use 'base';
@use 'layout';
// sass/base/_index.scss

@import 'reset';
@import 'typography';
// sass/base/_typography.scss

@use '~katex/dist/katex.css';
@use '~firacode/distr/fira_code.css';
// in webpack.config.js

{
  test: /\.s[ac]ss$/i,
  use: [
    'style-loader',
    'css-loader',
    {
      loader: 'postcss-loader', // Run postcss actions
      options: {
        plugins: function () { // postcss plugins, can be exported to postcss.config.js
          return [
            require('autoprefixer')
          ];
        }
      }
    },
    {
      loader: 'sass-loader',
      options: {
        // Prefer `dart-sass`
        implementation: require('sass'),
        sassOptions: {
          fiber: require('fibers'),
        },
      },
    },
  ],
}

@donnysim
Copy link
Contributor Author

donnysim commented Nov 8, 2019

@mitinarseny you could try removing the .css extension - @use "~katex/dist/katex" and it should find it. If it's a bug or not, I'm not sure. In addition, you'll most likely need to add resolve-url-loader because webpack resolves assets based on original source file so it won't find any assets inside the css file etc..

@donnysim
Copy link
Contributor Author

donnysim commented Nov 8, 2019

@mitinarseny
https://github.com/webpack-contrib/sass-loader/blob/master/src/importsToResolve.js#L42
seems like it's skipping css extensions because of node-sass bug.

@evilebottnawi is that check still necessary? node-sass actually imports the contents of the file rather than doing @import url(~css/some-css-module.css); if we remove the line, which is more inline with @use behavior. Or is it there because it shouldn't do it? There was a time where they deprecated imports of css files, but it seems they went back on it.

I don't see a way to get current implementation to skip it on dartSass or get the import type - @import or @use, any ideas or suggestions?

@mitinarseny
Copy link

you could try removing the .css extension - @use "~katex/dist/katex" and it should find it

Nope. It does not have any influence, it's just a recommendation according to sass docs. Anyway, this is solution which I came to, too:

In addition, you'll most likely need to add resolve-url-loader because webpack resolves assets based on original source file so it won't find any assets inside the css file etc..

And it works, but you should remove ~ and import like:

import 'katex/dist/katex';

BTW, @use should not use url(), according to sass docs.

@dietergeerts
Copy link

dietergeerts commented Jun 5, 2020

I'm currently investigating to start using "@use", and I wonder if it works if you have multiple scss imports from js files? So for example, I know solve the "output styling rules only once, even if shared across components" like this :

component-a.js

import './component-base.scss';
import './component-a.scss';

component-b.js

import './component-base.scss';
import './component-b.scss';

And because of this, the styling inside the "base" sass file will be included only once, and in the correct order (this is how I have been doing sass modules for quite some while, and it works so great for only having the things used in your bundle). If I would add this as an import in the individual sass files for the components, someone that imports both components will end up with duplicate styling rules in the result.

I saw '@use', and I wonder if this could remove this extra import in favor of having it in the scss file instead, which makes more sense? Because the sass-loader triggers for each separate scss import, I expect this not being the case, but I could be wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants