-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
Added the possibility to configure the StyleLoader via the method Enc… #715
Added the possibility to configure the StyleLoader via the method Enc… #715
Conversation
…ore.configureStyleLoader()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tooltonix,
Thank you for this PR.
Besides the few remarks below the implementation looks good, but we'll also need some test cases to make sure that it works and will still work in the future :)
Also I'm wondering if this should be added as a separate method or directly as a disableCssExtraction()
argument (since it is the only method that enables the loader)... what do you think? (not asking you to change it, I only want your opinion! Also, ping @weaverryan and @Kocal for the same question).
Hi 👋
Hum, I am bit divided on this point but I think a separate method is better. If we had use a
But if we have a
|
Due to internal requirements, we use a different style loader, which we install per yarn, so that the options of the previous style loader are no longer applicable. In the latest version 1.1.3 of the StyleLoader, the loader automatically inject source maps when previous loader emit them. It is correct that the method disableCssExtraction() activates the StyleLoader, but I would override the options with a separate method configureStyleLoader(), as it is more consistent for me in context and name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @tooltonix!
This looks good to me - and I agree with the 2 separate methods, but I'm happy we had the conversation - I think it will help us with some docs to show that these methods are connected.
Only minor comments. But we do need a functional test for this. See functional.js
- there is already a case that includes disableCssExtraction()
. We could duplicate that test case, maybe set the attributes
option to TESTING_ATTRIBUTES
and then assert that this string is present inside main.js
.
Cheers!
…xtraction' because they belong together(callback) {
Hi @weaverryan I'm sorry, but I haven't written any tests in Mocha yet. But I tried anyway and the test was successful, as long as I used the correct options of the style loader in version 1.1.3. I hope that the test is sufficient. Otherwise I ask for appropriate suggestions. |
Very excellent work! Thanks so much for this Patrick! |
…ore.configureStyleLoader()