Skip to content

Conversation

avaly
Copy link
Contributor

@avaly avaly commented Apr 4, 2017

What kind of change does this PR introduce?
Test cases: passing and failing

Did you add tests for your changes?
Yes, only tests.

Summary
Using style-loader and css-loader with local scoped identifiers and composes works as expected.

However when adding css-loaders getLocalIdent option, only some identifiers are properly passed through the custom getLocalIdent, while the identifiers from the composed files are still using the localIdentName option.

See the failing test local scope, composing, custom getLocalIdent

I submitted this PR here, because this is not reproducible when using only the css-loader.

@jsf-clabot
Copy link

jsf-clabot commented Apr 4, 2017

CLA assistant check
All committers have signed the CLA.

@michael-ciniawsky michael-ciniawsky self-assigned this Apr 18, 2017
@michael-ciniawsky michael-ciniawsky changed the title More test cases for usage with css-loader and composes test: more test cases for usage with css-loader and composes Apr 18, 2017
@michael-ciniawsky
Copy link
Member

@avaly Sry for the delay based on your description this sounds like a bug in css-loader instead?

@michael-ciniawsky michael-ciniawsky self-requested a review April 21, 2017 09:37
@michael-ciniawsky
Copy link
Member

The CI is also failing but it seems this is intentional for demonstration?

@avaly
Copy link
Contributor Author

avaly commented Apr 21, 2017

@michael-ciniawsky I'm not sure where the issue is between style-loader and css-loader. What's interesting is I can't reproduce this bug using only css-loader, only when throwing style-loader in the mix. I suspect it might be from how the options of css-loader are being handled by webpack when chaining the two loaders, since the getLocalIdent is a function, but not sure.

@avaly
Copy link
Contributor Author

avaly commented Apr 21, 2017

@michael-ciniawsky Sorry for opening the PR here, but I tried adding this type of failing tests for the css-loader repo, but I could not replicate the bug. I will open a bug in that repo to make this visible there as well.

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Apr 21, 2017

I also need to take a deeper look at it first aswell 😛, yep please open an issue in css-loader so i can keep track of that

Is this PR for demonstration purposes only or subject to eventually merge in the future ? 🙃

@michael-ciniawsky michael-ciniawsky removed their request for review April 21, 2017 12:28
@avaly
Copy link
Contributor Author

avaly commented Apr 21, 2017

I was looking into the css-loader issues now, and I found webpack-contrib/css-loader#389. It seems that the solution you suggested there (adding an ident) fixes this issue.

I've updated my branch and tests seem to pass. Feel free to merge it or not, though I feel it might be helpful for others who might run into this.

@michael-ciniawsky michael-ciniawsky changed the title test: more test cases for usage with css-loader and composes test: usage with css-loader and composes Apr 21, 2017
@michael-ciniawsky michael-ciniawsky merged commit 6ca068f into webpack-contrib:master Apr 21, 2017
@avaly avaly deleted the bug/css-loader-getLocalIdent branch April 30, 2017 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants