[develop] refactor: apply webpack-defaults by michael-ciniawsky · Pull Request #542 · webpack/css-loader

michael-ciniawsky · 2017-05-24T14:04:51Z

Refactor

apply webpack-defaults

Features

pass PostCSS AST directly to avoid 2x parsing (perf)
pass locals

Breaking

TODO

allow to filter certain url()'s (options.url {Array|Regex})
readd CSS Modules
readd loclas processing (camelCase as default, no option ?)
export CSS && locals as ES2015 Module

Discuss

No :locla :local() (Global Mode) ? (Recommended)
Readd cssnano or move to postcss-loader ?

cc @sullenor @TrySound

michael-ciniawsky · 2017-05-24T14:36:30Z

+
+      let locals;
+
+      if (meta && meta.locals) {


This would be where locals are added if postcss-loader passes them, needs to be replaced with the current implementation or ES2015 Module Export

import css, { className1, className2 } from './file.css' {String}, {Locals} import css, * as styles from 'file.css' // or import './file.css' {String} import * as styles from './file.css' {Locals}

@michael-ciniawsky I would prefer

import css, { className1, className2 } from './file.css' {String}, {Locals}

But honestly I do not even know 😄

😅 yeah.. it's a possiblity import * as styles from 'file.css' is the clear favourite 🏅

michael-ciniawsky · 2017-05-24T14:37:32Z

+
+  Promise.resolve()
+    .then(() => {
+      if (meta && meta.ast) return processor(ast, map, processOptions);


We can pass the PostCSS AST directly now to avoid a second parse of CSS

@michael-ciniawsky it be good, very good, more perf

alexander-akait · 2017-05-24T14:59:48Z

@michael-ciniawsky But what about the new loader? Seems this PR related to current implementation.

We have problems with perf and css-modules in current implementation. If i set modules: false, postcss plugins for css-modules still working (https://github.com/webpack-contrib/css-loader/blob/master/lib/processCss.js#L162), it is failed in perf. We should fix this in this PR.

alexander-akait · 2017-05-24T15:02:58Z

@@ -0,0 +1,8 @@
+{
+  "file": "alias",


Please don't use config as json, this does not make it possible to test the js functions and other expressions.

Don't remove tests, we must adapt them, otherwise we will get millions of regressions as in postcss-loader.

And frankly speaking, this approach to testing is not very convenient, we need to revise it.

Yep, this needs an update as in postcss-loader

alexander-akait · 2017-05-24T15:07:47Z

/cc @bebraw @d3viant0ne take part

michael-ciniawsky · 2017-05-24T15:17:22Z

@evilebottnawi I adpated the basic tests and a few are missing as the decision to move directly to new-loader was made I stopped working on this. If we start afresh with CSS Modules and PostCSS I will continue to port all tests and anyone can make PR against develop. This is just the baseline atm

michael-ciniawsky · 2017-05-24T15:19:49Z

modules are removed atm, but @sullenor what's to readd them with a different implementation, we definitely need to avoid :local :local() in 'global' mode and test perf in general with modules

michael-ciniawsky · 2017-05-24T15:21:45Z

The develop branch is a PoC, but we need a place to start and triage, either turns out bad or we can adapt what's in new-loader etc. and move on here

alexander-akait · 2017-05-24T15:26:52Z

@michael-ciniawsky seems we need good roadmap for css-loader, also decide
questions about @import and url() in new-css-loader.

michael-ciniawsky · 2017-05-24T15:35:12Z

Yep I take a look at this PR later and I think we should just start then, all outstanding things can get there own issue and PR for discussion and we have at least webpack-defaults and cleaner code base, everything else follows... 😛

joshwiens · 2017-05-24T20:52:19Z

Given defaults already exists in new-loader I don't know that it really makes sense to apply defaults to the existing css-loader. It really depends on what the plan is for the style related loader chain as a whole which hasn't been disseminated yet.

We have been asked to not do anything major with css-loader while Tobias is working on the new-loader and this definitely qualifies, ping him in slack and get his opinion on defaults as it relates to the existing css-loader.

mightyaleksey · 2017-05-25T03:59:02Z

+ *
+ * @return {cb} cb       Callback
+ */
+export default function loader (src, map, meta) {


I didn't know that loaders support the meta argument. What is it actually? :)

As far as I remember loaders supported a possibility to pass value to the next loader, which would be available as inputValue (https://webpack.github.io/docs/loaders.html#value).
Is meta a more explicit way to do that?

Yep this should work according to sokra, needs to be tested though :), the fourth argument of the callback can take a meta {Object}

function loader (src, map, meta) {// {String} | {Object} | {Object} .... .then(({ css, map, messages }) => cb(null, css, map, { messages }) }

mightyaleksey · 2017-05-25T04:06:23Z

+      let locals;
+
+      if (meta && meta.locals) {
+        locals = `exports.locals = ${meta.locals};`;


Should meta.locals be a string?
I actually thought that user would pass a plain object here, so it will be convenient to use it :)

Also, in the future versions the object may contain a prototype chain (css-modules/css-modules#147), so it would be necessary to make a proper serializer here.

No no, this is more of a placeholder yet :D :D. I was in the process of trying out how to ES2015 modulify this. That's where we need to start afresh and if CSSM tsay in css-loader there will be no meta locals, since this was originally intended to coma from postcss-loader. Any other ideas how we can do it a highly welcome 😛

let locals = [] Object.keys(meta.locals).forEach((key) => { locals.push(`export const ${key} = ${meta.locals[key]}`) }) locals = locals.join('\n')

JSON.stringify will be enough for the beginning :)
Actually, css-loader had an option, which camelized key names of the locals, so thats why it was a bit complicated and looks like some ppl still use it.

michael-ciniawsky · 2017-05-25T05:47:56Z

@d3viant0ne The develop branch is only a thing if CSSM stay in one way or the other, but since we don't really know how to handle CSS Modules (removal) atm and all the this and that won't move any further without some hands on code, we would like to try out the possibilities :). If we have certainty about CSSM we either ask Tobias the fuss here. develop can likely be 🔪 at the end of the day :D.

michael-ciniawsky · 2017-05-25T05:49:09Z

@sullenor @evilebottnawi Should we just merge this into develop so everyone can start to make PR's against develop ? There's plenty of work left 😛

mightyaleksey · 2017-05-25T06:29:52Z

I think the right solution will be to make a single postcss plugin, which will contain the necessary set of plugins which implement the css-modules spec and expose the necessary api. It will allow to embed (or to remove) it easily. Since our plan is not to make a major changes (as I understood @d3viant0ne right) this solution should fit here.

But there are some difficulties related to the css modules composition part. Composition (icss :import()) provides a possibility to load additional files. This part I would like to have also as a part of css-modules implementation (postcss plugin). However css-loader is almost a latest loader in the chain and for the proper behavior we need also to pass the loaded chunks through the remanning loaders in the chain to preprocess them.

/cc @TrySound

mightyaleksey · 2017-05-25T06:36:14Z

we need also to pass the loaded chunks through the remanning loaders in the chain to preprocess them

I guess importLoaders flag was designed for that purpose.

michael-ciniawsky · 2017-05-25T06:42:52Z

Yeah agree it would be the best to have a standalone plugin postcss-modules overhaul 🙃 ? + e.g result.messages['CSSM API here...']

importLoaders prefix @import with a query so it becomes a request on it's own => @import 'path/to/file.css' => importLoaders => require(-->./~css-loader?{}!./~postcss-loader?{}!<--path/to/file.css) the idea is good the implementation is bad 😊

joshwiens · 2017-05-25T08:54:29Z

@michael-ciniawsky - Tobias has a design for the style loader chain which he will be disseminating to all of us soon.

In the short term, the right solution is to do nothing until we know how css support in webpack is going to work, when it's going to land and how that effects the existing published loaders.

michael-ciniawsky self-assigned this May 24, 2017

michael-ciniawsky added priority: 2 (urgent) pr_action: Rebase semver: Major status: Blocked type: Refactor labels May 24, 2017

michael-ciniawsky added this to the 1.0.0 milestone May 24, 2017

refactor: apply webpack-defaults

e354715

michael-ciniawsky commented May 24, 2017

View reviewed changes

michael-ciniawsky added pr_action: Review and removed pr_action: Rebase labels May 24, 2017

michael-ciniawsky requested a review from alexander-akait May 24, 2017 14:43

alexander-akait reviewed May 24, 2017

View reviewed changes

michael-ciniawsky added status: Needs triage and removed status: Blocked labels May 24, 2017

michael-ciniawsky mentioned this pull request May 24, 2017

feat: add option to warn at runtime when non-existent properties are accessed #421

Closed

mightyaleksey reviewed May 25, 2017

View reviewed changes

michael-ciniawsky mentioned this pull request May 25, 2017

Add support for CSS Modules with explicit filename - [name].module.css facebook/create-react-app#2285

Merged

6 tasks

mightyaleksey mentioned this pull request May 25, 2017

Rename to postcss-modules-icss css-modules/postcss-icss#6

Closed

michael-ciniawsky closed this May 27, 2017

michael-ciniawsky removed priority: 2 (urgent) pr_action: Review semver: Major status: Needs triage type: Refactor labels May 27, 2017

michael-ciniawsky removed this from the 1.0.0 milestone May 27, 2017

Uh oh!

Conversation

michael-ciniawsky commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Refactor

Features

Breaking

TODO

Discuss

Uh oh!

michael-ciniawsky May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexander-akait May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait commented May 24, 2017

Uh oh!

michael-ciniawsky commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-ciniawsky commented May 24, 2017

Uh oh!

michael-ciniawsky commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexander-akait commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-ciniawsky commented May 24, 2017

Uh oh!

joshwiens commented May 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mightyaleksey May 25, 2017 • edited by michael-ciniawsky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky commented May 25, 2017

Uh oh!

michael-ciniawsky commented May 25, 2017

Uh oh!

mightyaleksey commented May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mightyaleksey commented May 25, 2017

Uh oh!

michael-ciniawsky commented May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-ciniawsky commented May 24, 2017 •

edited

Loading

michael-ciniawsky May 24, 2017 •

edited

Loading

alexander-akait commented May 24, 2017 •

edited

Loading

alexander-akait May 24, 2017 •

edited

Loading

michael-ciniawsky commented May 24, 2017 •

edited

Loading

michael-ciniawsky commented May 24, 2017 •

edited

Loading

alexander-akait commented May 24, 2017 •

edited

Loading

michael-ciniawsky May 25, 2017 •

edited

Loading

mightyaleksey May 25, 2017 •

edited by michael-ciniawsky

Loading

michael-ciniawsky May 25, 2017 •

edited

Loading

mightyaleksey commented May 25, 2017 •

edited

Loading

michael-ciniawsky commented May 25, 2017 •

edited

Loading