Add resolve options for imports from css files. #474

deAtog · 2018-12-20T22:01:07Z

These options change the default resolver functionality when import is used in css files.
This reduces the burden on the developer to locate styles provided by npm packages.

E.g. With these changes the follow become valid:

sass-test.scss:

// Resolution via "sass" property in package.json file in npm package
@import '~bootstrap';

// Resolution via file extension to css-test.css
@import 'css-test';

css-test.css

// Resolution via "style" property in package.json file in npm package
@import '~@fortawesome/fontawesome-free';

…import in sass files to locate a file to import based on the package.json file and to look for local files with the appropriate extentions when not specified.

…mport in css files to locate a file to import based on the package.json file and to look for local files with the appropriate extensions when not specified.

Lyrkan

Thanks for this PR @deAtog!

It looks fine to me and I can't see any real downside, however I think a test case is also required to make sure we won't break people projects if, for some reason, the resolving stops working in a future version.

deAtog · 2019-01-02T15:36:22Z

I'm not sure how to write tests for this. Maybe you can help with that aspect of it?

I can't think of any case where these changes would break an existing project as CSS imports currently have to be very explicit otherwise users end up importing a Javascript file in their CSS file. E.g. The main property in package.json for bootstrap is 'dist/js/bootstrap' which is a Javascript file. Thus without these changes, ~bootstrap in a CSS file resolves to a Javascript file.

Lyrkan · 2019-01-02T15:55:27Z

What I meant by "to make sure we won't break people projects" is that once this feature is introduced projects will be able to use things like @import '~bootstrap';.

Now imagine that in a future release (for instance when we're going to migrate to Webpack 5) this feature stops working. If we don't have any test that checks if the resolving works correctly we won't be able to know that something is broken... and that could cause unexpected errors in these projects.

As for how to test it... adding a functional test case that compiles a CSS/SCSS file with that kind of import should do the trick.

weaverryan · 2019-01-05T04:39:02Z

@deAtog I'm very interested in this change - I've noticed the "sass" and "style" properties on package.json in some packages, but couldn't figure out where it's used. Could you point me to some documentation or blog post or something about this? Basically, we try to stay as "standard" Webpack as possible, while making people's lives easier. We're 100% for moving towards standards, but less eager to move towards emerging or non-standard solutions. I don't have enough information about this yet to make a decision.

Thanks!

deAtog · 2019-01-07T19:38:49Z

I'm not sure the "style" and "sass" properties are a standard just yet, but they are definitely a convention. The next version of the sass-loader may include these defaults internally, but that change is currently blocked by a bug which causes the resolve options specified in the webpack-config to be ignored by the sass-loader. These changes add that functionality and is consistent with how the sass-loader may eventually work without these options.

weaverryan

Hi @deAtog!

Sorry for the delay! I just need you to help walk me through exactly how this works so that I understand and can merge it confidently.

Thanks!

weaverryan · 2019-02-02T18:47:42Z

fixtures/css/sass_package_import.scss

+// @import '~lib/test-pkg';
+
+// Importing without the tilde seems to work for webpack aliases
+@import 'lib/test-pkg';


Can you walk me through this process for how this is resolved? I don't quite see the full picture yet:

Webpack sees that we're loading the lib/test-pkg module

But, it doesn't know yet (right?) if this is loading a JS file (silly example, but technically correct), a JSON file, a font file or a CSS file. So, how does it know to look at the resolve key from the css rule?

When the sass-loader encounters 'lib/test-pkg' it asks Webpack to perform the resolution of this import and it does so using the process outlined in the Resolve configuration. Webpack will notice that lib is an alias to ./lib and that test-pkg is a directory under it which contains a package.json file. To determine which file to import, Webpack evaluates each value in the mainFields resolve option in order to determine if it has been specified in the package.json file. If the mainFields resolve option is not specified, Webpack defaults to browser, module, and then main when the target property is set to web,webworker, or is unspecified. If none of these properties exist in the package.json file, then Webpack will revert to using extension based resolution. In this case since 'lib/text-pkg' resolves to a directory, Webpack will look for a file in this directory using the names specified in mainFiles resolve option having an extension of one of the values specified in the extensions resolve option. The mainFiles resolve option defaults to index and the extensions resolve options defaults to ['.wasm', '.mjs', '.js', '.json']. With the options below specified, Webpack will instead look for index.scss, index.sass, and then index.css.

weaverryan · 2019-02-02T18:48:29Z

lib/config-generator.js

@@ -290,6 +294,10 @@ class ConfigGenerator {

        if (this.webpackConfig.useSassLoader) {
            rules.push({
+                resolve: {
+                    mainFields: ['sass', 'style', 'main'],
+                    extensions: ['.scss', '.sass', '.css']


What exactly does this do in the process? I know what resolve.extensions does at the root level - it allows you to omit the file extension. But what about here? If I say import 'foo-bar' where foo-bar is a module, what is the purpose of the extensions?

The resolve.extensions value overrides the value at the root level. When you say @import 'foo-bar' Webpack will first check for an alias called foo-bar. If it does not find an alias for foo-bar it will check each directory specified in resolve.modules for a directory called foo-bar. Assuming it finds that directory, it will then look to see if there is a package.json file for the module. If there is, it will use the resolve.mainFields setting to determine which file to import. If there is not then it will use the resolve.mainFiles in conjunction with resolve.extentions to determine what file to look for in that directory.

If you were to say @import './foo-bar' then Webpack would look in the current directory for a file named foo-bar with an extension of .scss, .sass, or .css in that order.

One more question: because this is being added to the sass loader, this resolution logic applies to files being imported from within a Sass file only, right? So, for example, if I were in foo.css (the earlier import), ./foo-barwould look forfoo-bar.cssbut notfoo-bar.scss` (because that rule only lives down here).

What about less & stylus below? Do those also need these options?

Yes, these are loader specific options. So @import "./foo-bar" in foo.css would only look for ./foo-bar.css and not foo-bar.scss.

These options can be specified for any loaders where appropriate so long as that loader uses Webpack to perform the import resolution.

deAtog · 2019-03-19T15:26:31Z

@Lyrkan any chance of getting this merged? Additionally, is there a better forum for discussing these sort of changes? My responses have gone unanswered for over a month and the last commit I made is now over two months old. Needless to say, the response time here is very sub-optimal to my current work plan. There are several improvements I'd like to make, but am starting to think it's not worth my time to get them accepted here.

Lyrkan · 2019-03-19T15:38:27Z

@deAtog Apart from the few linting issues (see the Travis build, the test job should however be solved after a rebase) I'm not against merging this PR, however I'd also like a 👍 from @weaverryan before doing so (since he expressed some concerns).

If you want to discuss there's an #encore channel on Slack (or feel free to send me a dm).

weaverryan

Sorry for the delay. Can you link also to the discussion about this in sass-loader about possibly making this the default in the future?

Thank you :)

weaverryan · 2019-03-25T23:33:27Z

lib/config-generator.js

@@ -290,6 +294,10 @@ class ConfigGenerator {

        if (this.webpackConfig.useSassLoader) {
            rules.push({
+                resolve: {
+                    mainFields: ['sass', 'style', 'main'],
+                    extensions: ['.scss', '.sass', '.css']


One more question: because this is being added to the sass loader, this resolution logic applies to files being imported from within a Sass file only, right? So, for example, if I were in foo.css (the earlier import), ./foo-barwould look forfoo-bar.cssbut notfoo-bar.scss` (because that rule only lives down here).

What about less & stylus below? Do those also need these options?

weaverryan · 2019-03-29T13:11:48Z

Thanks for your patience @deAtog - this is a very cool feature! Could you please open a documentation pull request so that people will know about it?

…llingsworth, weaverryan) This PR was merged into the master branch. Discussion ---------- Add resolve options for imports from css files. These options change the default resolver functionality when import is used in css files. This reduces the burden on the developer to locate styles provided by npm packages. E.g. With these changes the follow become valid: sass-test.scss: ```css // Resolution via "sass" property in package.json file in npm package @import '~bootstrap'; // Resolution via file extension to css-test.css @import 'css-test'; ``` css-test.css ```css // Resolution via "style" property in package.json file in npm package @import '~@fortawesome/fontawesome-free'; ``` Commits ------- 7895f63 fixing CS 9525e7a Merge branch 'master' into resolve 382bc5f Add functional test for css imports via package.json style property b9e42be Add functional test for sass imports via package.json sass property 0f9657f Specify resolve option to be used with the css-loader. This allows @import in css files to locate a file to import based on the package.json file and to look for local files with the appropriate extensions when not specified. b54ec7b Specify resolve option to be used with the sass-loader. This allows @import in sass files to locate a file to import based on the package.json file and to look for local files with the appropriate extentions when not specified.

David Ellingsworth added 2 commits December 20, 2018 15:41

Specify resolve option to be used with the sass-loader. This allows @…

b54ec7b

…import in sass files to locate a file to import based on the package.json file and to look for local files with the appropriate extentions when not specified.

Specify resolve option to be used with the css-loader. This allows @i…

0f9657f

…mport in css files to locate a file to import based on the package.json file and to look for local files with the appropriate extensions when not specified.

Lyrkan requested changes Jan 2, 2019

View reviewed changes

David Ellingsworth added 2 commits January 7, 2019 14:11

Add functional test for sass imports via package.json sass property

b9e42be

Add functional test for css imports via package.json style property

382bc5f

weaverryan reviewed Feb 2, 2019

View reviewed changes

weaverryan reviewed Mar 25, 2019

View reviewed changes

weaverryan added 2 commits March 29, 2019 08:50

Merge branch 'master' into resolve

9525e7a

fixing CS

7895f63

weaverryan merged commit 7895f63 into symfony:master Mar 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add resolve options for imports from css files. #474

Add resolve options for imports from css files. #474

deAtog commented Dec 20, 2018 •

edited

Lyrkan left a comment

deAtog commented Jan 2, 2019

Lyrkan commented Jan 2, 2019 •

edited

weaverryan commented Jan 5, 2019

deAtog commented Jan 7, 2019

weaverryan left a comment

weaverryan Feb 2, 2019

deAtog Feb 6, 2019

weaverryan Feb 2, 2019

deAtog Feb 7, 2019

weaverryan Mar 25, 2019

deAtog Mar 28, 2019

deAtog commented Mar 19, 2019

Lyrkan commented Mar 19, 2019

weaverryan left a comment

weaverryan Mar 25, 2019

weaverryan commented Mar 29, 2019

Add resolve options for imports from css files. #474

Add resolve options for imports from css files. #474

Conversation

deAtog commented Dec 20, 2018 • edited

Lyrkan left a comment

Choose a reason for hiding this comment

deAtog commented Jan 2, 2019

Lyrkan commented Jan 2, 2019 • edited

weaverryan commented Jan 5, 2019

deAtog commented Jan 7, 2019

weaverryan left a comment

Choose a reason for hiding this comment

weaverryan Feb 2, 2019

Choose a reason for hiding this comment

deAtog Feb 6, 2019

Choose a reason for hiding this comment

weaverryan Feb 2, 2019

Choose a reason for hiding this comment

deAtog Feb 7, 2019

Choose a reason for hiding this comment

weaverryan Mar 25, 2019

Choose a reason for hiding this comment

deAtog Mar 28, 2019

Choose a reason for hiding this comment

deAtog commented Mar 19, 2019

Lyrkan commented Mar 19, 2019

weaverryan left a comment

Choose a reason for hiding this comment

weaverryan Mar 25, 2019

Choose a reason for hiding this comment

weaverryan commented Mar 29, 2019

deAtog commented Dec 20, 2018 •

edited

Lyrkan commented Jan 2, 2019 •

edited