Skip to content

feat: add options.matchImportLoadersByOrigin#544

Closed
delijah wants to merge 1 commit intowebpack:masterfrom
delijah:match-import-loaders-by-origin
Closed

feat: add options.matchImportLoadersByOrigin#544
delijah wants to merge 1 commit intowebpack:masterfrom
delijah:match-import-loaders-by-origin

Conversation

@delijah
Copy link
Copy Markdown

@delijah delijah commented May 29, 2017

What kind of change does this PR introduce?
This PR introduces a new config option called "matchImportLoadersByOrigin" config option. With this config option set to true, all imports (@import and composes) are resolved matching webpack loaders by it's origin instead of just using the same loader chain, that was used for it's containing file. importLoaders config option is not needed anymore, when using matchImportLoadersByOrigin.

#287 is closely related and probably even the more solid version, but without config option. I do not understand why this never has been merged since it would solve so many issues.

Did you add tests for your changes?
If it is needed, i can add tests. First of all i would like to see, if my changes make sense.

If relevant, did you update the README?
Yes.

Summary
This should resolve the following issues:
#286
#131
css-modules/css-modules#110
css-modules/css-modules#170

Does this PR introduce a breaking change?
No

@jsf-clabot
Copy link
Copy Markdown

jsf-clabot commented May 29, 2017

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 29, 2017

Codecov Report

Merging #544 into master will decrease coverage by 0.51%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #544      +/-   ##
==========================================
- Coverage   98.65%   98.13%   -0.52%     
==========================================
  Files          10       10              
  Lines         371      375       +4     
  Branches       89       91       +2     
==========================================
+ Hits          366      368       +2     
- Misses          5        7       +2
Impacted Files Coverage Δ
lib/loader.js 97.53% <50%> (-2.47%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a6b17d...7e99ea0. Read the comment docs.

@michael-ciniawsky michael-ciniawsky changed the title Implemented matchImportLoadersByOrigin config option feat: add options.matchImportLoadersByOrigin May 29, 2017
@michael-ciniawsky michael-ciniawsky self-assigned this May 29, 2017
@michael-ciniawsky michael-ciniawsky added this to the 0.29.0 milestone May 29, 2017
@michael-ciniawsky
Copy link
Copy Markdown
Contributor

michael-ciniawsky commented May 29, 2017

@delijah Could we add a prop to importLoaders instead, or default to this behaviour if importLoaders isn't set by the user ? The changes don't really justify to introduce a new option :D 😛 . Also this comment bugs me, we need to clearify what he meant by that first :)

@mightyaleksey
Copy link
Copy Markdown

@michael-ciniawsky should it be handled by resolve.extentions?
https://webpack.js.org/configuration/resolve/#resolve-extensions

@mightyaleksey
Copy link
Copy Markdown

ah, sorry, read it wrong. Actually importLoaders with proper number should solve it (you should set it to the number of loaders, that are after css-loader in the chain).

@delijah
Copy link
Copy Markdown
Author

delijah commented May 30, 2017

@sullenor i think a lot of things are working somehow. But using the same loader (let's assume a loader rule created for handling css) for a completely different file type (let's assume sass) just feels wrong.

I just tried to reconstruct those errors, but i was not able to. Seems like there was something changed since those issues were created.

So no real problems, but i think this importLoaders config option still does make no sense. Why not just using standard webpack behaviour and use the defined loader rules from webpack.config?

What do you think?

@michael-ciniawsky
Copy link
Copy Markdown
Contributor

michael-ciniawsky commented May 30, 2017

entry.css

@import './style.css';

.className { color: red;  }
style-loader <= css-loader <= postcss-loader 

after css-loader

/* importPrefix === 'css!postcss!' */
@import './style.css'  => require('${importPrefix}./style.css') 

Without the importPrefix (importLoaders) the @import wont't be transpiled to my knowlegde, that's why it was added. The possibility to make a new request and it respects module.rules for that request would make importLoaders obsolete (witch would be a good thing) I'm not 💯 on this either

const css = [
  [module.id, css, map, ...] // @import
  [module.id, css, map, ...] // entry.css
]

Your change is to omit the prefix, but isn't that the default already ? I'm missing something here ? Please elaborate further on this :)

@michael-ciniawsky
Copy link
Copy Markdown
Contributor

michael-ciniawsky commented Jun 1, 2017

@delijah Would this work, if importLoaders is set for other @imports ? I don't think this solves the issue with wrong request in composes in case it's composed from a different file format and other @import's relying on importLoaders wouldn't work anymore? It would require additional logic to omit the importPrefix for files where it isn't needed (How to determine this?)

Copy link
Copy Markdown
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need tests

Copy link
Copy Markdown
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also no new feature in current implementation, only bugfixes and perf.
See: #542 (comment)
See: #542 (comment)

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 this pull request may close these issues.

5 participants