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

Behaviour change between 7.1 and 7.2 (and later) #773

Closed
pedro-w opened this issue Oct 10, 2019 · 14 comments · Fixed by #823
Closed

Behaviour change between 7.1 and 7.2 (and later) #773

pedro-w opened this issue Oct 10, 2019 · 14 comments · Fixed by #823

Comments

@pedro-w
Copy link

pedro-w commented Oct 10, 2019

As discussed in readthedocs/sphinx_rtd_theme#808 there appears to be a change in the way files are resolved which doesn't seem to correspond to a documented change in sass-loader. It may be a bug in sass-loader or a bug in the way the project that depends on sass-loader is configured. To simplify I have made a minimal project which shows this.
https://github.com/pedro-w/sass-test

  • Operating System: Debian 10.1 Buster
  • Node Version: v10.16.3
  • NPM Version: 6.9.0
  • webpack Version: 4.30.0
  • sass-loader Version: 7.x (various)

Expected Behavior

Example project should compile on all 7.x versions

Actual Behavior

In 7.1 and earlier it is OK, for 7.2 and later an error occurs

ERROR in ./src/theme.sass
Module not found: Error: Can't resolve '../fonts/fontawesome-webfont.eot' in '/home/pedro-w/Projects/sass-test/src'
 @ ./src/theme.sass 5:38-81
 @ multi ./src/index.js ./src/theme.sass

Code

https://github.com/pedro-w/sass-test/blob/d8b458430fc04fe7ef24d083027ac5356d4bc00d/webpack.prod.js#L1-L44

How Do We Reproduce?

Clone the repo above and attempt to build. Change pinned version to 7.1 and the project will now build (note: the output is not useful, it's just for testing)

@alexander-akait
Copy link
Member

@pedro-w do you try latest 8 version?

@pedro-w
Copy link
Author

pedro-w commented Oct 10, 2019

Failed with error

ERROR in ./src/theme.sass
Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
ValidationError: Invalid options object. Sass Loader has been initialised using an options object that does not match the API schema.
 - options has an unknown property 'includePaths'. These properties are valid:
   object { implementation?, sassOptions?, prependData?, sourceMap?, webpackImporter? }
    at validate (/private/tmp/sass-test/node_modules/schema-utils/dist/validate.js:50:11)
    at Object.loader (/private/tmp/sass-test/node_modules/sass-loader/dist/index.js:36:28)
 @ multi ./src/index.js ./src/theme.sass theme[1]

but that is not the same error, it's an API change, isn't it?
I will see if I can get it to work with version 8.

@alexander-akait
Copy link
Member

Just move includePaths inside sassOptions

@pedro-w
Copy link
Author

pedro-w commented Oct 10, 2019

Made it work with version 8, by putting options under the sassOptions key

diff --git a/webpack.prod.js b/webpack.prod.js
index f8b4fae..a21bf74 100644
--- a/webpack.prod.js
+++ b/webpack.prod.js
@@ -17,9 +17,11 @@ module.exports = {
                     {
                         loader: "sass-loader?indentedSyntax",
                         options: {
-                            includePaths: [
-                                "node_modules/font-awesome/scss"
-                            ]
+                            sassOptions: {
+                                includePaths: [
+                                    "node_modules/font-awesome/scss"
+                                ]
+                            }
                         }
                     }
                 ]

Now I get

ERROR in ./src/theme.sass
Module not found: Error: Can't resolve '../fonts/fontawesome-webfont.eot' in '/private/tmp/sass-test/src'
 @ ./src/theme.sass 5:38-81
 @ multi ./src/index.js ./src/theme.sass

as before.

Maybe the webpack config was just wrong before and only worked on <=7.1 by luck?
[edit] Sorry just seen your post above - that would have saved me some time!!

@alexander-akait
Copy link
Member

@pedro-w to be honestly i think it is not sass-loader, we unified logic for resolving url in css-loader, i.e. ~ works work the same way with modules and without, some developers have problem with their old code (webpack-contrib/css-loader#914), anyway i will investigate this in near future

@pedro-w
Copy link
Author

pedro-w commented Oct 11, 2019

Alright, I've found that if I change

@import font-awesome

to

@import ~font-awesome/scss/font-awesome

it will work, but we did have "node_modules/font-awesome/scss" in Webpacks's includePaths (link) - so I am assuming that somewhere between 7.1 and 7.2, sass-loader (or its dependencies) stopped using these include paths or changed the way it uses them.
Does that make sense? is that a possible explanation?

@pedro-w
Copy link
Author

pedro-w commented Oct 14, 2019

Just a bit more, I really don't understand this very well.
The 'breaking change' was commit 28f1884 - before this it was OK and after not. It seems that just adding the word 'style' to mainFields is what does it.

So, for people wanting to @import stuff from node dependencies, either:

  1. They use the ~ and a path, and don't bother with includePaths since it doesn't do anything
  2. This really is a bug and should be fixed.

What do you suggest? I can work on it some more if you think it is a bug.

@alexander-akait
Copy link
Member

Looks like a bug, I still did not investigate this deeply, but if you have ideas on how to fix it, you can send a PR 👍

@pedro-w
Copy link
Author

pedro-w commented Oct 20, 2019

Just a quick note to say that I haven't disappeared; I've done a bit more work on this but no conclusion yet. May not be a problem with sass-loader at all, but in enhanced-resolve

@alexander-akait
Copy link
Member

@pedro-w don't worry it is in my todo list in near future

@alexander-akait
Copy link
Member

alexander-akait commented Oct 24, 2019

@pedro-w i found problem, sass-loader since 7.2.0 start support sass/style/main field in package.json (https://github.com/webpack-contrib/sass-loader/blob/master/src/index.js#L44), but font-awesome use css https://github.com/FortAwesome/Font-Awesome/blob/v4.7.0/package.json#L5 so you got css instead scss (and variable doesn't work), right solution is adding sass field to font-awesome package, but I'm pretty sure that it will be impossible to do, since this is an old version.

Solutions:

  • Use full path to scss - @import "~font-awesome/scss/font-awesome" (prefer).
  • Don't use webpackimporter (i.e. false) https://github.com/webpack-contrib/sass-loader#webpackimporter and use includePaths.
  • Send a PR to font-awesome package and add sass field.
  • Using @use with latest sass (dart-sass)
  • Maybe we should fix it edge case on sass-loader side.

Unfortunately, historically css/sass/less is not modular but webpack try to add support for that, but it is has limitation like that.

Why it is happens? The node-sass and sass (dart-sass) provide importer ability to resolve @import and sass-loader use this ability, but importer works before the node-sass/sass tries to process @imports themselves so we return to node-sass/sass path to css ( node_modules/css/font-awesome.css, based on field from package.json).

I think we can try to fix it, need check what file in import doesn't exists in includePaths. Here similar problem with includePaths #767

@pedro-w
Copy link
Author

pedro-w commented Oct 25, 2019

Very interesting, thanks for taking the time to look into this. At the moment they have implemented the first solution in your list. Do you know if upgrading fontawesome from v4 to v5 would also fix it?

@alexander-akait
Copy link
Member

Found solution

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 a pull request may close this issue.

2 participants