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

Initial ES6 modules support #402

Closed
wants to merge 10 commits into from
Closed

Initial ES6 modules support #402

wants to merge 10 commits into from

Conversation

simlrh
Copy link

@simlrh simlrh commented Jan 18, 2017

What kind of change does this PR introduce?

Imports and exports changed to use the native ES6 module system.

importLoaders option changed to allow importLoaders to run after the css-loader (to allow babel to run on imported css modules).

Did you add tests for your changes?

Yes, updated tests.

If relevant, did you update the README?

Yes

Summary

ES6 exports work with tree shaking, which makes it possible to track which CSS classes are used and remove dead CSS (ie in a plugin that fires prior to minification).

Does this PR introduce a breaking change?

This version requires a new style-loader to consume the exports correctly, and the same for any loader expecting output from css-loader. A PR for a compatible style-loader is being posted along with this one.

Other information

"@import" statements import the default object from imported css. When using "composes:" only the symbols used are imported.

CSS classes are exported separately as camel case variables. An object containing all classes is exported as the default. Classes that clash with JS reserved words are not exported separately but are included in the default. The default object has a toString function which returns the CSS itself.

In addition an object called $css is exported which contains the css source & sourcemap and a list of imported css media query information to help style-loader.

Resolves #210

@jsf-clabot
Copy link

jsf-clabot commented Jan 18, 2017

CLA assistant check
All committers have signed the CLA.


return res;
}, []).join(",\n");
}, []);
exportJs.push("export default { " + symbols.join(", ") + " };");
Copy link
Member

Choose a reason for hiding this comment

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

It should not default export the key, but export each symbol separate. Best emit an

export var className = "class-name";

per class name.

The CSS itself should be exported via default export.

export default css;

Copy link
Member

Choose a reason for hiding this comment

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

This means class name always need to be converted to camelCase from dashed-case

Copy link
Author

@simlrh simlrh Jan 18, 2017

Choose a reason for hiding this comment

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

It does both, exports each separate symbol (line 28) but also exports all symbols as properties on one default object (line 33).

This was done for compatibility with the CSS modules spec where eg import styles from './styles.css'; is expected to give you such an object. Exporting the CSS source by default makes more sense in terms of a loader but wouldn't allow that behaviour. It also allows dashed-case inside the default object, although not in the separated symbols.

lib/loader.js Outdated

var js = "import { $cssLoader as $cssLoader" + hash + " } from '" + importUrl + "';\n";
if (symbols.length) {
js = js + "import { " + symbols.join(", ") + " } from '" + importUrl + "';\n";
Copy link
Member

Choose a reason for hiding this comment

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

Instead of '" + importUrl + "' always use JSON.stringify(importUrl), elsewise if breaks for chars that need to be escaped.

.babelrc Outdated
@@ -0,0 +1,3 @@
{
"presets": ["es2015"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? You specify the preset in the tests specifically.

Copy link
Author

Choose a reason for hiding this comment

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

No sorry, removed

@simlrh
Copy link
Author

simlrh commented Jan 19, 2017

It should not default export the key, but export each symbol separate. ... The CSS itself should be exported via default export.

I've tried to address this by adding a toString function which returns the CSS itself. Hopefully this is an acceptable solution as the CSS is available but it also works the way CSS Modules expect.

Another tradeoff is skipping separate export of classnames which clash with JS reserved words (eg .class), but these are still exported in the default object.

@codecov-io
Copy link

codecov-io commented Jan 19, 2017

Codecov Report

Merging #402 into master will increase coverage by 0.49%.
The diff coverage is 99.21%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #402      +/-   ##
=========================================
+ Coverage    98.4%   98.9%   +0.49%     
=========================================
  Files           9      11       +2     
  Lines         314     365      +51     
  Branches       71      78       +7     
=========================================
+ Hits          309     361      +52     
+ Misses          5       4       -1
Impacted Files Coverage Δ
lib/loader.js 100% <100%> (ø) ⬆️
lib/css-base.js 100% <100%> (+2.85%) ⬆️
lib/compile-exports.js 100% <100%> (ø) ⬆️
lib/camelCase.js 100% <100%> (ø)
lib/compile-imports.js 100% <100%> (ø)
lib/localsLoader.js 97.95% <96.77%> (-2.05%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6da7e90...f8fbf02. Read the comment docs.

@joshwiens
Copy link
Member

joshwiens commented Jan 21, 2017

@simlrh - Can you pick up master when you have a second, it has a bunch of applicable Travis updates.

@simlrh
Copy link
Author

simlrh commented Jan 23, 2017

Regarding using tree shaking to remove unused rules from the CSS, I've implemented this as a plugin called webpack-dead-css, demo project here.

@simlrh
Copy link
Author

simlrh commented Jan 26, 2017

Changed importLoader behaviour to allow loaders to run before and after css-loader on imported modules. Necessary so that babel-loader can be applied to the ES6 output. Eg:

    {
        loader: 'css-loader',
        query: {
            importLoaders: {
                beforeCssLoader: [ 'postcss-loader' ],
                afterCssLoader: [{
                    loader: 'babel-loader',
                    query: {
                        presets: [ ['es2015', {modules: false}] ]
                    }
                }]
            }
        }
    }

@simlrh
Copy link
Author

simlrh commented Jan 30, 2017

Not sure now that changing importLoader behaviour is the best solution.

If loaders are going to return ES2015 modules then babel-loader will need to be added to the top of the loader chain for every file type, as well as importLoaders on any imported CSS. (I had 3 babel-loader invocations in the demo project for this PR). And webpack 2 can handle ES2015 anyway, so there's no actual need to transpile in a loader any more. I think the better solution is to forget babel-loader and run babel during "optimize-chunk-assets" like UglifyJsPlugin.

See babel-webpack-plugin for this behaviour.

@simlrh
Copy link
Author

simlrh commented Feb 6, 2017

FYI the CLA check has been broken since the repo moved organisation so I can't access it to sign again, but I did previously sign the agreement: #402 (comment)

@michael-ciniawsky
Copy link
Member

@simlrh This PR is definitely stalled which is shame tbh, contains a lot of the fixes we recently merged or working on. If you still have the motivation to rebase this PR and refactor all the superseded parts out feel free to do so and let my know :)

⚠️ Note that ES2015 Modules support will be part of the upcomingwebpack-defaultsupgrade alongside with some other breaking changes.

Otherwise I will close this PR and for still outstanding issues that were adressed by this PR feel free to send PR after the webpack-defaults upgrade, I ensure it will not get stalled again 😛

@michael-ciniawsky
Copy link
Member

Closing for now but feel free to reopen :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants