philipbelesky / inter-ui

A npm package for distributing the Inter font family created by Rasmus Andersson.
https://github.com/rsms/inter/
SIL Open Font License 1.1
111 stars 6 forks source link

SASS error since 3.12.1 #13

Closed carloscarnero closed 4 years ago

carloscarnero commented 4 years ago

I'm using Webpack to process and use Inter and a bunch of other resources. Up to 3.12.0, I had a .scss file with just

@import "~inter-ui/inter-hinted.scss";

which worked as expected. Starting with 3.12.1, I'm getting this error when I run Webpack:

ERROR in ./src/scss/inter.scss
Module build failed (from ./node_modules/mini-css-extract-plugin/dist/loader.js):
ModuleBuildError: Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
SassError: Invalid CSS after "@include default":
  expected 1 selector or at-rule, was ".all;"
    on line 5 of node_modules/inter-ui/inter-hinted.scss
    from line 4 of <SNIP>/src/scss/inter.scss
>> @include default.all;
   ---------^

(<SNIP>/src/scss/inter.scss is my integration, where I load Inter.) I have seen that this version brought some changes, including the possibility to include specific weights, which is absolutely great. I tried the .scss with the documented example:

@use "~inter-ui/default" as inter-ui with (
    $inter-font-path: "~inter-ui/Inter (web latin)"
);
@include inter-ui.weight-400;
@include inter-ui.weight-700;

and got a similar error:

ERROR in ./src/scss/inter.scss
Module build failed (from ./node_modules/mini-css-extract-plugin/dist/loader.js):
ModuleBuildError: Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
SassError: Invalid CSS after "@include inter-ui":
  expected 1 selector or at-rule, was ".weight-400;"
    on line 20 of <SNIP>src/scss/inter.scss
>> @include inter-ui.weight-400;
   ---------^

What am I missing?

philipbelesky commented 4 years ago

Hey Carlos thanks for getting in touch. I confess that I'm not super familiar with this area of SASS syntax and have been meaning to look into it properly since the PR for this landed. My best guess is maybe the version of SASS you are using doesn't support this import syntax, but maybe @manuelmeister may have an idea?

manuelmeister commented 4 years ago

@carloscarnero The sass version would be my guess as well. I used the @use syntax instead of the @import syntax because sass advises doing so: https://sass-lang.com/documentation/at-rules/import

In the error message, it can't handle the namespacing.

Your version of the sass-implementation may not support this. What version does your sass-loader have?

carloscarnero commented 4 years ago

Your version of the sass-implementation may not support this. What version does your sass-loader have?

Currently I'm set with sass-loader version 8.0.2 and the implementation is node-sass version 4.13.1.

carloscarnero commented 4 years ago

If you notice the opening comment, you'll see that I had

@import "~inter-ui/inter-hinted.scss";

and following @manuelmeister's suggestion about @use vs @import I changed the above into

@use "~inter-ui/inter-hinted.scss";

and that builds! Now, I just have to see why

@use "~inter-ui/default" as inter-ui with (
    $inter-font-path: "~inter-ui/Inter (web latin)"
);
@include inter-ui.weight-400;
@include inter-ui.weight-700;

still gives the error.

EDIT: I got excited too soon. While the build succeeds, it will not process the font sources and it actually puts @use "~inter-ui/inter-hinted.scss" into the generated CSS. What I'm going to do, is to build a reduced test case with just enough bits for Inter, as there are too many moving parts in what I currently have.

manuelmeister commented 4 years ago

Strange. You used the @import correctly as you did want to include the whole inter-hinted. Please keep me updated. The problem could also be with the ~ in @use "~inter-ui/inter-hinted.scss"

I'm sorry for the inconvenience!

carloscarnero commented 4 years ago

I have created the simplest case that closely matches what I have in production, except that only Inter is integrated. I have found that using sass (Dart) will correctly build the assets, while node-sass will not (with the aforementioned errors.)

manuelmeister commented 4 years ago

Yes. I just found out that @use is only supportet on sass-dart:

Compatibility: Dart Sass since 1.23.0, LibSass ✗, Ruby Sass ✗ Only Dart Sass currently supports @use. Users of other implementations must use the @import rule instead.

carloscarnero commented 4 years ago

Oh! Apparently LibSass @use support, sass/libsass#3051, is on hold waiting for sass/sass#1094 (an issue opened in 2014.) In the mean time, I'll try to migrate to sass instead of node-sass? Thank you all!

EDIT: FYI, I successfully migrated my production workflow to Dart (with the added convenience that one of the upstream projects that I use, Video.js, also migrated to Dart a while ago.)

philipbelesky commented 4 years ago

Thanks for helping out here @manuelmeister. I could look into different import mechanisms if there is need, but it sounds like migration away from node-sass is a broadly good path.

manuelmeister commented 4 years ago

Nice! @philipbelesky yes I think, it would make sense to add this to the requirements with a link to the issues that @carloscarnero mentioned.

philipbelesky commented 4 years ago

Update: have added a note to the README in 765bcf707b2ab814d35f37a599ca79e21c00c874