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

Problem with :external on SASS. #568

Closed
hemmxwxsoo opened this issue Mar 14, 2019 · 9 comments
Closed

Problem with :external on SASS. #568

hemmxwxsoo opened this issue Mar 14, 2019 · 9 comments

Comments

@hemmxwxsoo
Copy link

hemmxwxsoo commented Mar 14, 2019

I'm using modular-css with SASS, Webpack. When I use :external, sass-loader causes an error.

{
    test: /\.scss$/,
    loaders: [
        "@modular-css/webpack/loader",
         'sass-loader'
    ],
},
.child {
    width: 100px;
    height: 100px;
    background-color: red;
}
:external(app from "./app.scss"):hover .child { // It works when I disable sass-loader
    background-color: yellow;
}

Expected Behavior

GIF
I want to make :external work with sass-loader.

Current Behavior

When I run a server with npm run start, I got an error like

ERROR in ./components/child.scss
Module build failed (from ../node_modules/sass-loader/lib/loader.js):

:external(app from "./app.scss"):hover .child {
                  ^
      Invalid CSS after "...ternal(app from": expected selector, was '"./app.scss"):hover'
      in C:\Users\abc\Desktop\modular-css-with-sass-test\src\components\child.scss (line 8, column 20)
 @ ./components/Child.jsx 25:0-33 42:19-24
 @ ./components/App.jsx
 @ ./index.js
 @ multi ../node_modules/webpack-dev-server/client?http://localhost:8080 ../node_modules/webpack/hot/dev-server.js react-hot-loader/patch webpack-dev-server/client?http://localhost:8080 webpack/hot/only-dev-server ./index.js

Possible Solution

Steps to Reproduce (for bugs)

  1. git clone https://github.com/hemmxwxsoo/modular-css-with-sass-test
  2. cd modular-css-with-sass-test
  3. npm install
  4. npm run start

Webpack config's path : ./configs/webpack/common.js
components path : ./src/components

Context

Your Environment

Executable Version
modular-css 22.2.0
npm --version 6.4.1
node --version 10.15.1
OS Version
Windows 10 1809
@jquense
Copy link
Contributor

jquense commented Jul 9, 2019

this is libsass, not being able to properly parse these pseudo's. perhaps there is an alternative syntax that might work here that plays nicely with sass? not being able to use with sass is a blocker for us :/

@tivac
Copy link
Owner

tivac commented Jul 9, 2019

modular-css should be removing all traces of its custom syntax from the CSS. If you run it through modular-css before passing it to SASS does it still break?

@tivac
Copy link
Owner

tivac commented Jul 9, 2019

REPL

This uses most of the custom modular-css syntax and it all appears to be removed correctly, leaving behind only valid CSS.

@jquense
Copy link
Contributor

jquense commented Jul 9, 2019

You wouldn't run modular css first generally, same with css-modules. The sass/less/stylus compilation runs first, outputting css which you turn then process. In all likelihood module-css wouldn't be able to even run on sass files since it's not valid css

@tivac
Copy link
Owner

tivac commented Jul 9, 2019

Oops, that seems obvious in retrospect. Is there really no way to ignore unknown pseudos in SASS? Can you run something postcss-based first using postcss-scsss as the parser?

I'm not thrilled about the idea of changing the syntax or supporting both the current version and an alternative. It matches the existing :local and :global values from CSS Modules as well as the original proposal in css-modules/css-modules#147.

@tivac
Copy link
Owner

tivac commented Jul 10, 2019

Oh, there might be another solution though it's a bit gross. You could write a postcss plugin to convert a SASS-compatible version of :external() into the original modular-css syntax. If you use that with the before hook it should be totally fine to rewrite parts of the file before modular-css does any real work on it.

Just a theory, but should be totally possible. I get it if that's a bridge too far but figured I'd kick the idea out there just in case it helped.

@jquense
Copy link
Contributor

jquense commented Sep 16, 2019

btw trying to get this fixed upstream in libsass sass/libsass#2990

@tivac
Copy link
Owner

tivac commented Mar 31, 2020

Looks like this got fixed upstream thanks to @jquense!

@tivac tivac closed this as completed Mar 31, 2020
@jquense
Copy link
Contributor

jquense commented Mar 31, 2020

for others, if using node-sass, it's gonna be a while before it makes it into a release :/ That project seems like it's years behind the current sass version

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

No branches or pull requests

3 participants