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

Enhancement: Change SCSS include syntax to support the latest webpack sass-loader #3253

Closed
blowery opened this issue Jul 27, 2018 · 3 comments

Comments

@blowery
Copy link

blowery commented Jul 27, 2018

The current sass files include their dependencies the standard sass way, minus the file extension. That works just fine for most sass libraries, but in webpack@4 it doesn't work well.

The latest sass loader uses webpack's module resolution algorithm, and as a result, when you have a scss file and a js file with the same name, it will prefer the JS over the SCSS and the import breaks.

The workaround is to always include scss files with the extension so there's no ambiguity about which file to process. @import 'foo' becomes @import 'foo.scss'.

I'll try to add a simple repro soon, but I'm currently on a train. 🚋

@blowery
Copy link
Author

blowery commented Jul 27, 2018

OK, I worked up a repro repo at https://github.com/blowery/importing-material-css

However, I think I was wrong about needing to add extensions to make things work reliably. I got things working without modifying the SCSS imports inside of the published material packages. So, yay? Yay.

There were two ways for a client to make it work, as shown in https://github.com/blowery/importing-material-css/tree/using-resolve-extensions and https://github.com/blowery/importing-material-css/tree/using-extensions-on-import

First, the client needs to set up the sass-loader as shown in the existing material docs, with an options: { includePaths: [ 'node_modules' ] } set.

If the client wants to include sass files with extension, nothing further is necessary. Just import '@material/button/mdc-button.scss and they're done.

If the client wants to include sass files without specifying the extension, like import '@material/button/mdc-button', they have to add .scss to the array of extensions to check when resolving modules in the webpack config.

So I don't think anything is necessary from material's end. Maybe a doc update for webpack 4, but that's it.

@blowery blowery closed this as completed Jul 27, 2018
@kfranqueiro
Copy link
Contributor

kfranqueiro commented Jul 27, 2018

@blowery would MDC Web's own intra- and inter-package imports still cause this issue if the extensions array isn't set up properly? It might still be a good idea for us to update our imports to be explicit if it increases compatibility.

@blowery
Copy link
Author

blowery commented Jul 27, 2018

@kfranqueiro it occurs to me that Dojo already did this, but I digress. ;)

Resolution appears to work out without the extensions array. It seems like once you're inside a sass file, different rules apply, but ... maybe not? Resolution inside sass-loader is a bit exciting.

webpack-contrib/sass-loader#296 and webpack-contrib/sass-loader#307 shed some light on what's going on. The end of 296 especially.

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

2 participants