feat: add option.base #164

ilanc · 2017-01-13T09:56:07Z

…loader/issues/163

What kind of change does this PR introduce?
bugfix / workaround for #163

Did you add tests for your changes?
no

If relevant, did you update the README?
no

Summary
This is a workaround. It allows the user to manually specify base id's for each bundle when multiple bundles are used on a page. The user would for have to manually specify the order in various webpack.configs like so:
dll1.js
loaders:[ { test: /\.css$/, loader: 'style-loader!css-loader' } ]
dll2.js
loaders:[ { test: /\.css$/, loader: 'style-loader?cssBase=1000!css-loader' } ]
app.js
loaders:[ { test: /\.css$/, loader: 'style-loader?cssBase=2000!css-loader' } ]

see #163

Does this PR introduce a breaking change?
no - the user has to opt in - there's no impact if the user doesn't specify option.cssBase

Other information

jsf-clabot · 2017-01-13T09:56:08Z

All committers have signed the CLA.

zojeda · 2017-01-17T15:47:27Z

+1 we are having the same issue, this worked fine for us, hope this PR gets approved

michael-ciniawsky · 2017-03-15T22:59:39Z

@ilanc Please close and reopen the PR to trigger the CLA Bot again and signed the CLA please 😛

michael-ciniawsky · 2017-03-15T23:00:39Z

Also add a test please

ilanc · 2017-03-16T06:58:29Z

Hi there @michael-ciniawsky I've re-signed the CLA. How do I add a test in this case? Did you see the related bug report btw? https://github.com/ilanc/style-loader-bug

ekulabuhov

Please add an explanation of the new option to the README.

ekulabuhov · 2017-03-16T16:15:29Z

@ilanc a good unit test here would be to re-create the exact problem you had and show that your fix indeed solves it. Have you looked at the test/basicTest.js? They exercise webpack with different loader setups and configurations in memory. Tell me if you're having any problems setting it up.

ilanc · 2017-03-18T09:06:47Z

Thanks @ekulabuhov I've just gone on holiday I'll look into it in 10 days time

ilanc · 2017-04-03T08:33:42Z

Hi chaps, I've added the notes to the README. I don't have the tests in my branch - looks like these were added in Feb. Do you want me to merge (or perhaps rebase) my changes off the latest head?

bebraw · 2017-04-03T08:39:54Z

@ilanc Please rebase. A lot has been going on. 😄

michael-ciniawsky · 2017-04-25T00:46:32Z

@ilanc I will try to take a look at it, but I haven't much experience using the DllPlugin

@bebraw @d3viant0ne @evilebottnawi anyone used to DllPlugin and cpuld help @ilanc with this? 🙃

alexander-akait · 2017-04-25T09:07:34Z

@michael-ciniawsky i have pure knowledge about Dll 😄 /cc @ilanc i am right that https://github.com/ilanc/style-loader-bug repo contain example bug?

ilanc · 2017-04-25T13:50:43Z

@michael-ciniawsky @bebraw @d3viant0ne @evilebottnawi

Yes https://github.com/ilanc/style-loader-bug gives you a good example of DllPlugin in use. It's theoretical though - i.e. it's not a real app. See this article if you want to see a real use case.

The style-loader-bug repo is a good minimal example for use as a unit test. However as I mentioned you're going to have to jump through a lot of hoops if you want to have a unit test in the style-loader project which depends on functionality in the core webpack repo (DllPlugin functionality lives in core webpack IIRC).

CssBase is only useful with DllPlugins - there's no css clash between existingStyles and webpack css - I thought of trying this as a unit test but there's no clash so it's not a useful unit test for cssBase. The clash only occurs when you load a DllPlugin.

Load up the style-loader-bug repo and follow the build instructions in the readme.md then run in chrome with dev tools open and single step through the code. There are comments in the .html file showing what's going wrong.

michael-ciniawsky · 2017-04-25T13:59:33Z

cssBase doesn't introduce any possible side effects for other users when not using/setting it right (I need to ask so naively 🙃 )? If so and it works with DllPlugin in a sample project, we could leave the test(s) for it in style-loader out, if impossible to unit test atm. Although I intend to run tests through webpack in the near future, but that's not a concern of yours, since it isn't in place yet 😛

michael-ciniawsky · 2017-04-25T14:03:30Z

addStyles.js

 	var styles = [];
 	var newStyles = {};
 	for(var i = 0; i < list.length; i++) {
 		var item = list[i];
-		var id = item[0];
+		var id = item[0] + (+(options.cssBase || 0));


- var id = item[0] + (+(options.cssBase || 0)) + var id = options.cssBase ? item[0] + options.cssBase : item[0]

michael-ciniawsky · 2017-04-25T14:08:02Z

README.md

+#### `cssBase`
+This setting is primarily used as a workaround for [css clashes](https://github.com/webpack-contrib/style-loader/issues/163) when using one or more [DllPlugin](https://robertknight.github.io/posts/webpack-dll-plugins/)'s.  `cssBase` allows you to prevent either the *app*'s css (or *DllPlugin2*'s css) from overwriting *DllPlugin1*'s css by specifying a css module id base which is greater than the range used by *DllPlugin1* e.g.:
+* webpack.dll1.config.js
+    * `loaders:[ { test: /\.css$/, loader: 'style-loader!css-loader' } ]`


Please use webpack 2 syntax here 😛

{ test: /\.css$/, use: [ { loader: 'style-loader', options: { cssBase: 1000 } }, 'css-loader' ] }

michael-ciniawsky · 2017-04-25T14:16:09Z

@ilanc Could we use e.g options.cssBase * 1e3 to make the options terser

- { loader: 'style-loader`, options: { cssBase: 1000 } }
+ { loader: 'style-loader`, options: { cssBase: 1 } }

Naming

cssBase => base
cssBase => idBase

The intention to bump the module id range by e.g n * 1e3 when using DllPlugin isn't fully clear imho :)

ilanc · 2017-04-27T15:03:13Z

I don't think options.cssBase * 1e3 makes any meaningful difference - i arbitrarily choose 1000. If you only require 5 css files in Dll1 then Dll2 probably only needs a cssBase of 6* or higher in order to avoid clashing with Dll1's css modules.

I prefer cssBase over the other suggestions because it's a css specific problem - the others are a bit generic.

* Of course the number of modules in a Dll plugin isn't exactly equal to the number of require statements so using 1000 is just being cautious.

michael-ciniawsky · 2017-04-28T01:32:47Z

I don't think options.cssBase * 1e3 makes any meaningful difference - i arbitrarily choose 1000. If you only require 5 css files in Dll1 then Dll2 probably only needs a cssBase of 6* or higher in order to avoid clashing with Dll1's css modules.

👍

I prefer cssBase over the other suggestions because it's a css specific problem - the others are a bit generic.

Disagreeing here since the css prefix doesn't add any meaning imho (nothing else then CSS is 'processed' by this loader anyways), so we can keep it terse and generic. Please rename to either id || base || range with base as the favorite, but enterily free choice between those three options and good to fo then 😛

michael-ciniawsky · 2017-04-28T01:33:22Z

Please also close and reopen the PR to trigger the CLA Bot again and sign the CLA

ilanc · 2017-04-30T07:34:55Z

@michael-ciniawsky cool I think it's ready to go - looks like the CLA bot did it's thing.

michael-ciniawsky

@ilanc Thx

This reverts commit e4ac886.

ilanc · 2017-04-30T10:28:48Z

:) happy to help

SpaceK33z changed the title ~~add option.cssBase = workaround for https://github.com/webpack/style-…~~ add option.cssBase = workaround for #163 Jan 13, 2017

joshwiens force-pushed the master branch from afd5db9 to 2630c0f Compare March 15, 2017 04:22

michael-ciniawsky assigned ekulabuhov and michael-ciniawsky Mar 15, 2017

michael-ciniawsky requested review from bebraw, ekulabuhov and joshwiens March 15, 2017 22:59

michael-ciniawsky added type: Feature pr_action: Review pr: tests labels Mar 15, 2017

michael-ciniawsky approved these changes Mar 15, 2017

View reviewed changes

michael-ciniawsky mentioned this pull request Mar 15, 2017

style-loader removes .css when webpack.DllPlugin's used and module.id's overlap #163

Closed

bebraw approved these changes Mar 16, 2017

View reviewed changes

ekulabuhov suggested changes Mar 16, 2017

View reviewed changes

michael-ciniawsky removed their assignment Mar 16, 2017

Ilan Copelyn added 3 commits April 4, 2017 16:14

add option.cssBase = workaround for webpack-contrib#163

4fde3cd

cast options.cssBase to number

e908a1f

Added cssBase info to README.md

15c60b2

ilanc force-pushed the master branch from 4c70c3f to 15c60b2 Compare April 4, 2017 14:16

michael-ciniawsky added type: Feature and removed pr: feature labels Apr 23, 2017

michael-ciniawsky added this to the 0.17.0 milestone Apr 23, 2017

michael-ciniawsky added the priority: 3 (required) label Apr 25, 2017

michael-ciniawsky requested changes Apr 25, 2017

View reviewed changes

Ilan Copelyn added 2 commits April 27, 2017 17:04

webpack2 syntax

841ef15

javascript syntax

7a72bdd

michael-ciniawsky changed the title ~~feat: add option.cssBase~~ feat: add option.base Apr 28, 2017

michael-ciniawsky mentioned this pull request Apr 28, 2017

feat: add option.transform #146

Merged

renamed options.cssBase to options.base

e37e7cb

ilanc closed this Apr 30, 2017

ilanc reopened this Apr 30, 2017

michael-ciniawsky approved these changes Apr 30, 2017

View reviewed changes

michael-ciniawsky merged commit e4ac886 into webpack-contrib:master Apr 30, 2017

michael-ciniawsky added a commit that referenced this pull request Apr 30, 2017

Revert "feat: add option.base (#164)"

99bd60f

This reverts commit e4ac886.

michael-ciniawsky removed priority: 3 (required) status: tests labels Apr 30, 2017

michael-ciniawsky modified the milestone: 0.18.0 May 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add option.base #164

feat: add option.base #164

ilanc commented Jan 13, 2017

jsf-clabot commented Jan 13, 2017 •

edited

Loading

zojeda commented Jan 17, 2017

michael-ciniawsky commented Mar 15, 2017

michael-ciniawsky commented Mar 15, 2017

ilanc commented Mar 16, 2017

ekulabuhov left a comment •

edited

Loading

ekulabuhov commented Mar 16, 2017

ilanc commented Mar 18, 2017

ilanc commented Apr 3, 2017

bebraw commented Apr 3, 2017

michael-ciniawsky commented Apr 25, 2017 •

edited

Loading

alexander-akait commented Apr 25, 2017

ilanc commented Apr 25, 2017

michael-ciniawsky commented Apr 25, 2017

michael-ciniawsky Apr 25, 2017

michael-ciniawsky Apr 25, 2017

michael-ciniawsky commented Apr 25, 2017

ilanc commented Apr 27, 2017

michael-ciniawsky commented Apr 28, 2017

michael-ciniawsky commented Apr 28, 2017

ilanc commented Apr 30, 2017

michael-ciniawsky left a comment

ilanc commented Apr 30, 2017

feat: add option.base #164

feat: add option.base #164

Conversation

ilanc commented Jan 13, 2017

jsf-clabot commented Jan 13, 2017 • edited Loading

zojeda commented Jan 17, 2017

michael-ciniawsky commented Mar 15, 2017

michael-ciniawsky commented Mar 15, 2017

ilanc commented Mar 16, 2017

ekulabuhov left a comment • edited Loading

Choose a reason for hiding this comment

ekulabuhov commented Mar 16, 2017

ilanc commented Mar 18, 2017

ilanc commented Apr 3, 2017

bebraw commented Apr 3, 2017

michael-ciniawsky commented Apr 25, 2017 • edited Loading

alexander-akait commented Apr 25, 2017

ilanc commented Apr 25, 2017

michael-ciniawsky commented Apr 25, 2017

michael-ciniawsky Apr 25, 2017

Choose a reason for hiding this comment

michael-ciniawsky Apr 25, 2017

Choose a reason for hiding this comment

michael-ciniawsky commented Apr 25, 2017

Naming

ilanc commented Apr 27, 2017

michael-ciniawsky commented Apr 28, 2017

michael-ciniawsky commented Apr 28, 2017

ilanc commented Apr 30, 2017

michael-ciniawsky left a comment

Choose a reason for hiding this comment

ilanc commented Apr 30, 2017

jsf-clabot commented Jan 13, 2017 •

edited

Loading

ekulabuhov left a comment •

edited

Loading

michael-ciniawsky commented Apr 25, 2017 •

edited

Loading