Skip to content
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

feat: add option.transform #146

Merged
merged 14 commits into from
Apr 30, 2017
Merged

Conversation

brafdlog
Copy link
Contributor

I added the option to pass the name of a function that can make changes to the css before adding it to the html. The function can return false, in which case the css won't be loaded at all.
The relevant use cases for this are changes that that need context that is not available at build time.

For example:

  1. (partial) support for css variables in IE (which is the use case I currently need)
  2. Conditional stylesheet loading (issue Conditional Stylesheet #130)
  3. A similar function can be used for issue Do something after styles have been updated #37 (perhaps a postCssAdded callback)

These things were very hard (impossible?) to do without having a hook that gives control of the css when it is loaded into the browser.

My initial thought was to add another loader but since this transformation needs to run at runtime I need to take css as input and output javascript. So I can't put it before the style loader (which expects css, not js) and cant put it after the style loader (because then I would have to somehow parse it's javascript and change it which is quite brittle and not very elegant)

There is one thing that I don't like about this solution and that is that you need the transformCssOnLoad function to be available on the global scope for this to work. I could not think of a better way to support this, if there is a better idea I would love to hear it.

@brafdlog
Copy link
Contributor Author

@sokra What do you think?

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

Namings + Please add Docs for this

addStyles.js Outdated
@@ -143,6 +143,20 @@ function createLinkElement(options) {
function addStyle(obj, options) {
var styleElement, update, remove;

// If a transformCssOnLoad function was defined, run it on the css
if (options.transformCssOnLoad && obj.css) {
Copy link
Member

Choose a reason for hiding this comment

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

options.transformCssOnLoad =>options.transform

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose this name to make it clear that this transformation occurs at load time. I think this name makes it very clear what this function does. Do you think transform is better?

Copy link
Contributor

Choose a reason for hiding this comment

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

transformCssOnLoad sounds fine to me. transform alone does not communicate the intent.

Copy link
Member

Choose a reason for hiding this comment

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

kk. runtime ? transformOnLoad?

Copy link
Contributor

Choose a reason for hiding this comment

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

transformOnLoad sounds like a good compromise. Not too long but it also communicates something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

transformOnLoad and transformCssOnLoad both sound good.

Copy link
Member

Choose a reason for hiding this comment

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

@brafdlog Yep the sole transform is to jaded, personally in favour of transformOnLoad, the more precise and short the better 😛 . I will quit bikesheding about naming from this point, choose based on majority and preference :)

addStyles.js Outdated
@@ -143,6 +143,20 @@ function createLinkElement(options) {
function addStyle(obj, options) {
var styleElement, update, remove;

// If a transformCssOnLoad function was defined, run it on the css
if (options.transformCssOnLoad && obj.css) {
var transformationFunction = self[options.transformCssOnLoad];
Copy link
Member

Choose a reason for hiding this comment

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

var tranformationFunction => const/let transformation

Copy link
Contributor

@bebraw bebraw left a comment

Choose a reason for hiding this comment

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

This needs docs + tests to cover the functionality. Good after that.

@ekulabuhov
Copy link
Member

ekulabuhov commented Mar 6, 2017

Isn't that what loaders are for - transformation? Why put a loader inside a loader? Wouldn't that be very confusing?

addStyles.js Outdated
if (options.transformCssOnLoad && obj.css) {
var transformationFunction = self[options.transformCssOnLoad];
if (!transformationFunction) {
throw new Error("A function named " + options.transformCssOnLoad + " needs to be available on the global scope.");
Copy link
Member

Choose a reason for hiding this comment

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

A function on global scope seems dirty. Is it possible to pass a function instead of a name here?

@bebraw
Copy link
Contributor

bebraw commented Mar 6, 2017

@ekulabuhov If I'm interpreting right, there's a coupling problem since addStyles is something loaded indirectly. That said, if it's possible to push the idea to a separate loader outside of this project, that would be better.

@michael-ciniawsky
Copy link
Member

The relevant use cases for this are changes that that need context that is not available at build time.

@ekulabuhov Let's wait for a response, I'm also not 💯 yet

Copy link
Member

@joshwiens joshwiens left a comment

Choose a reason for hiding this comment

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

Much like the href PR, this feels like something that should be extracted, not included.

Personally, i'd go the route a creating a simple & focused loader and then chaining. Given we now have a simple way to spin that up & a home for all of them, spinning this off into it's own loader is far less a major undertaking than it once was.

@brafdlog
Copy link
Contributor Author

brafdlog commented Mar 6, 2017

@ekulabuhov @d3viant0ne

My initial thought was to add another loader but since this transformation needs to run at runtime I need to take css as input and output javascript. So I can't put it before the style loader (which expects css, not js) and cant put it after the style loader (because then I would have to somehow parse it's javascript and change it which is quite brittle and not very elegant)

Am I missing something here? How would I do this with another loader?

@joshwiens
Copy link
Member

@brafdlog - You aren't missing anything with style-loader in it's current state. At the moment, there is no good way to extract this but at the same time, it really should be.

My point, before we just hammer this home I would prefer to try and find a way to not shoehorn a loader into a loader. I get what you are trying to do and why, we just need to find a better way to accomplish it.

@ekulabuhov
Copy link
Member

@brafdlog - @d3viant0ne explained it pretty well. Just to clarify, I do think it's a great feature to have. But I, as a webpack user, wouldn't expect to find it in a style-loader.

I wonder, could you solve these issues using refs use() / unuse() ? Or would it be too cumbersome?

@michael-ciniawsky
Copy link
Member

Ok naysayers how would a solution with a different loader look like ? 😛

@bebraw
Copy link
Contributor

bebraw commented Mar 7, 2017

The tricky bit is that addStyles.js is a script loaded by style-loader. Maybe the question is actually would it be possible to add some kind of an extension point there? This might help with other functionality too.

@ekulabuhov
Copy link
Member

@michael-ciniawsky: npm i magical-runtime-loader -D :)

But seriously, is it possible to have 2 pitch loaders in one chain?

@michael-ciniawsky
Copy link
Member

Add the option to inject a addStylesRuntime.js/addStylesContext.js file, but I'm fine with adding a transform/runtime function to addStyles.js

@michael-ciniawsky
Copy link
Member

But seriously, is it possible to have 2 pitch loaders in one chain?

@ekulabuhov ? 🙃

@ekulabuhov
Copy link
Member

ekulabuhov commented Mar 7, 2017

@michael-ciniawsky https://webpack.github.io/docs/loaders.html#pitching-loader
If I understand correctly, style-loader works by injecting code rather than data. Is it possible to wrap style-loader code within imaginary runtime-transform-loader?

Also found a way to pass an object instead of query string to a loader: https://webpack.github.io/docs/how-to-write-a-loader.html#programmable-objects-as-query-option

@liady
Copy link
Contributor

liady commented Mar 9, 2017

@michael-ciniawsky @brafdlog We can achieve the separation of concerns by doing the following:

  1. Build a new dedicated 'runtime-css-transform' loader - that receives raw css as input and returns a js function (stringified) that, when invoked, returns transformed css.
  2. Extend the style-loader to receive a css string or a stringified js function (that returns css). If it receives a function - it runs it (on runtime, not build time) and operates as usual on the returned css.

Then a user can apply them in this order:
raw css => css-loader => runtime-css-transform-loader (that returns a function)=> style-loader (that runs this function)

What do you think?

(BTW, we can make the 'runtime-css-transform' loader generic, to operate on any arbitrary string, not just css)

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Mar 13, 2017

fixes #152

@michael-ciniawsky
Copy link
Member

@brafdlog @bebraw @d3viant0ne @ekulabuhov @liady I still think it isn't a bad idea to support passing a transform/runtime function to style-loader directly, we need to find a final agreement on this. Please vote with reactions on this comment ( 👍 / 👎 )

@michael-ciniawsky michael-ciniawsky changed the title feat: support runtime transformation of css feat: add option.cssTransform Apr 19, 2017
@brafdlog
Copy link
Contributor Author

@bebraw @ekulabuhov @d3viant0ne Are there any other changes we need to make?

README.md Outdated
```js
module.exports = function(originalCss) {
// Here we can change the original css
const transformed = css.replace('.classNameA', '.classNameB');
Copy link
Contributor

Choose a reason for hiding this comment

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

originalCss over css so that the code works.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or you can fix the other way around. Then it's consistent with the code below.

Copy link
Member

Choose a reason for hiding this comment

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

css > originalCss please 😛

@michael-ciniawsky michael-ciniawsky added this to the 0.17.0 milestone Apr 23, 2017
@michael-ciniawsky michael-ciniawsky changed the title feat: add option.cssTransform feat: add option.transform Apr 28, 2017
@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Apr 28, 2017

Please rename to options.transform I requested the same in #164 it's simply terser and the css prefix doesn't add any special meaning since this loader only 'processes' CSS anyways.

transform: (css) => {...}

Should be self explanatory and the option is well documented, finally good to go after that 😛

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

@brafdlog Thx

@michael-ciniawsky michael-ciniawsky merged commit 1c3943f into webpack-contrib:master Apr 30, 2017
@michael-ciniawsky michael-ciniawsky modified the milestone: 0.18.0 May 8, 2017
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.

7 participants