Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix: split async CSS correctly #546

Closed
wants to merge 28 commits into from
Closed

fix: split async CSS correctly #546

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
77daf83
refactor: pass a unique compiler name to get child compilation (#483)
sokra Jun 11, 2017
92e4349
chore(package): Update peerDeps & webpack devDep
joshwiens Jun 11, 2017
cdfccb4
Squashed commit of the following:
BowlingX Jun 20, 2017
3e78452
refactor: Apply webpack-defaults (#542)
joshwiens Jun 21, 2017
4880afb
Merge remote-tracking branch 'base/feature/webpack3'
BowlingX Jun 21, 2017
5df5d09
fix(tests): fixed test to work with webpack 3, adjusted dependencies
BowlingX Jun 21, 2017
b202f4a
only remove dependencies that have loaders defined
BowlingX Jun 21, 2017
292e217
refactor: Apply webpack-defaults (#542)
joshwiens Jun 21, 2017
28171b2
refactor: Chunk.modules deprecation warning
joshwiens Jun 23, 2017
1730d46
chore(release): 3.0.0-beta.0
joshwiens Jun 23, 2017
5d0c28f
fix: Distribute schema with package
joshwiens Jun 24, 2017
715b1dd
chore(release): 3.0.0-beta.1
joshwiens Jun 24, 2017
1ef755a
chore: Update install docs
joshwiens Jun 24, 2017
84a0328
chore(release): 3.0.0-beta.2
joshwiens Jun 24, 2017
b33e006
chore(package): Update deps minor versions
joshwiens Jun 24, 2017
d4a0c23
chore(release): 3.0.0-beta.3
joshwiens Jun 24, 2017
10721f5
chore: Update changelog for beta 1 & 2
joshwiens Jun 24, 2017
1175e63
Merge remote-tracking branch 'base/feature/webpack3'
BowlingX Jun 26, 2017
fb4eb9b
fixed merge conflicts
BowlingX Jun 26, 2017
400ed49
Merge branch 'master-remote'
BowlingX Jul 21, 2017
143760a
fixed tests
BowlingX Jul 21, 2017
163191a
Merge remote-tracking branch 'base/master'
BowlingX Nov 29, 2017
2ba04eb
adjusted tests
BowlingX Nov 29, 2017
3ef2916
adjusted tests
BowlingX Nov 29, 2017
8403a27
fixed code style, moved clone to helpers
BowlingX Nov 29, 2017
3aed4df
code style fixes
BowlingX Mar 18, 2018
f1a8ce2
moved schema files to src, adjusted build.
BowlingX Mar 18, 2018
edd39a1
Merge remote-tracking branch 'upstream/master'
BowlingX Apr 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

66 changes: 58 additions & 8 deletions src/index.js
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@ import { ConcatSource } from 'webpack-sources';
import async from 'async';
import loaderUtils from 'loader-utils';
import validateOptions from 'schema-utils';
import NormalModule from 'webpack/lib/NormalModule';
import ExtractTextPluginCompilation from './lib/ExtractTextPluginCompilation';
import OrderUndefinedError from './lib/OrderUndefinedError';
import {
Expand All @@ -26,7 +27,7 @@ class ExtractTextPlugin {
if (isString(options)) {
options = { filename: options };
} else {
validateOptions(path.resolve(__dirname, '../schema/plugin.json'), options, 'Extract Text Plugin');
validateOptions(path.resolve(__dirname, './schema/plugin.json'), options, 'Extract Text Plugin');
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Author

Choose a reason for hiding this comment

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

./schema is not resolved correctly when I build the branch from source. Not sure how the build process is, but without changing the path it does not compile.

Copy link

Choose a reason for hiding this comment

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

This should be reverted and the schema folder moved back to where it was.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally move schema/Plugin.json && schema/loader.json from /schema to /src/Plugin.json && /src/loader.json and add --copy-files to the build script in package.json please.

- schema
src
||– index.js 
||– Plugin.json
||- loader.js
||- loader.json
|
|-package.json

}
this.filename = options.filename;
this.id = options.id != null ? options.id : ++nextId;
Expand All @@ -40,6 +41,17 @@ class ExtractTextPlugin {
return { loader: require.resolve('./loader'), options };
}

static cloneModule(module) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this in https://github.com/webpack-contrib/extract-text-webpack-plugin/blob/master/src/lib/helpers.js

Copy link
Author

Choose a reason for hiding this comment

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

sure

return new NormalModule(
module.request,
module.userRequest,
module.rawRequest,
module.loaders,
module.resource,
module.parser,
);
}

applyAdditionalInformation(source, info) {
if (info) {
return new ConcatSource(
Expand Down Expand Up @@ -88,7 +100,7 @@ class ExtractTextPlugin {
if (Array.isArray(options) || isString(options) || typeof options.options === 'object' || typeof options.query === 'object') {
options = { use: options };
} else {
validateOptions(path.resolve(__dirname, '../schema/loader.json'), options, 'Extract Text Plugin (Loader)');
validateOptions(path.resolve(__dirname, './schema/loader.json'), options, 'Extract Text Plugin (Loader)');
Copy link

Choose a reason for hiding this comment

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

Should also be reverted.

Copy link
Author

@BowlingX BowlingX Nov 29, 2017 •
edited

Choose a reason for hiding this comment

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

If I run the tests with ../schema I get:

ENOENT: no such file or directory, open '/home/david/Projekte/extract-text-webpack-plugin/schema/loader.json'

Copy link

@danez danez Nov 30, 2017 •
edited

Choose a reason for hiding this comment

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

Because you moved the schema folder into the src folder, but @michael-ciniawsky wants to have them in a different location anyway, see other comment.

}
let loader = options.use;
let before = options.fallback || [];
Expand Down Expand Up @@ -126,8 +138,10 @@ class ExtractTextPlugin {
const filename = this.filename;
const id = this.id;
let extractedChunks;
let toRemoveModules;
Copy link
Member

Choose a reason for hiding this comment

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

\n

compilation.plugin('optimize-tree', (chunks, modules, callback) => {
extractedChunks = chunks.map(() => new Chunk());
toRemoveModules = [];
Copy link
Member

Choose a reason for hiding this comment

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

\n

chunks.forEach((chunk, i) => {
const extractedChunk = extractedChunks[i];
extractedChunk.index = i;
Expand All @@ -145,25 +159,38 @@ class ExtractTextPlugin {
const extractedChunk = extractedChunks[chunks.indexOf(chunk)];
const shouldExtract = !!(options.allChunks || isInitialOrHasNoParents(chunk));
chunk.sortModules();
async.forEach(chunk.mapModules(c => c), (module, callback) => { // eslint-disable-line no-shadow
async.forEach(chunk.mapModules((c) => { return c; }), (module, callback) => { // eslint-disable-line no-shadow, arrow-body-style
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Author

Choose a reason for hiding this comment

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

can be changed to c => c, properly a missed merge

let meta = module[NS];
if (meta && (!meta.options.id || meta.options.id === id)) {
const wasExtracted = Array.isArray(meta.content);
if (shouldExtract !== wasExtracted) {
module[`${NS}/extract`] = shouldExtract; // eslint-disable-line no-path-concat
compilation.rebuildModule(module, (err) => {
const newModule = ExtractTextPlugin.cloneModule(module);
newModule[`${NS}/extract`] = shouldExtract; // eslint-disable-line no-path-concat
Copy link
Member

Choose a reason for hiding this comment

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

\n

compilation.buildModule(newModule, false, newModule, null, (err) => {
if (err) {
compilation.errors.push(err);
return callback();
}
meta = module[NS];
meta = newModule[NS];
Copy link
Member

Choose a reason for hiding this comment

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

\n

const identifier = module.identifier();
Copy link
Member

Choose a reason for hiding this comment

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

\n

// Error out if content is not an array and is not null
if (!Array.isArray(meta.content) && meta.content != null) {
err = new Error(`${module.identifier()} doesn't export content`);
err = new Error(`${identifier} doesn't export content`);
compilation.errors.push(err);
return callback();
}
if (meta.content) { extractCompilation.addResultToChunk(module.identifier(), meta.content, module, extractedChunk); }
if (meta.content) {
Copy link
Member

Choose a reason for hiding this comment

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

\n (before)

extractCompilation.addResultToChunk(identifier, meta.content, module, extractedChunk);
if (toRemoveModules[identifier]) {
toRemoveModules[identifier].chunks.push(chunk);
} else {
toRemoveModules[identifier] = {
module: newModule,
moduleToRemove: module,
chunks: [chunk],
};
}
}
callback();
});
} else {
Expand Down Expand Up @@ -191,6 +218,29 @@ class ExtractTextPlugin {
callback();
});
});

compilation.plugin('optimize-module-ids', (modules) => {
modules.forEach((module) => {
const data = toRemoveModules[module.identifier()];
Copy link
Member

Choose a reason for hiding this comment

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

\n

if (data) {
const oldModuleId = module.id;
const newModule = ExtractTextPlugin.cloneModule(module);
newModule.id = oldModuleId;
newModule._source = data.module._source; // eslint-disable-line no-underscore-dangle
Copy link
Member

Choose a reason for hiding this comment

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

\n

data.chunks.forEach((chunk) => {
chunk.removeModule(data.moduleToRemove);
Copy link
Member

Choose a reason for hiding this comment

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

\n

const deps = data.moduleToRemove.dependencies;
Copy link
Member

Choose a reason for hiding this comment

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

\n

deps.forEach((d) => {
if (d.module && d.module.loaders.length > 0) {
chunk.removeModule(d.module);
}
Copy link
Author

Choose a reason for hiding this comment

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

About this, I realized that style-loader dependencies get removed if I don't check if loaders are defined here. I think this might properly not save enough, I should check for the actual setup loader here.

Copy link

Choose a reason for hiding this comment

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

I've been thinking about the best way to handle CSS in dynamic chunks. What i've come up with is this: import() needs to request both the js + the css.

I.e. 2 requests, rather than what I was doing before with extract-css-chunks-webpack-plugin where I made 2 js files (one with style-loader and another called no_css.js without style-loader). That adds considerably to the compilation time, as was reported by several users.

As far as HMR, this simplifies implementation--it's just one way to support, and it's guaranteed you longer need to worry about not getting HMR when generating stylesheets.

The most important thing however is this: having a different setup in production than you have in development is a bad idea (i.e. where in development css is served in js via style-loader, and during production via external stylesheets). Having the same setup (external stylesheets) means going to production has less unforeseen bottlenecks.

If it works in development, it works in production.

Developers new to Webpack expend a lot of wasted energy figuring this stuff out. I.e. fumbling around with file hashes, only embedding a stylesheet in production (whose file hash can be hard to find), etc. It's a frustrating stage in your project, especially when you thought you were done.

Since HMR works on stylesheets, it makes you think why was CSS ever injected from javascript? To save novice developers from pasting a <link /> tag in addition to main.js? Perhaps it's just legacy because that's how style loader was built first, or EWTP wasn't around in the beginning.

I think we gotta go back to square one. Maybe this is part of @sokra 's "masterplan."

In short, it's a small harmless babel plugin that changes import() to Promise.all([import(), fetchCss()]).

And now this plugin is easier than ever. It insures there is a matching CSS file per chunk, no matter whether it's named or dynamic. And it uses a very straightforward mechanism to reload CSS files corresponding to each module:

if (module.hot) {
	module.hot.accept();
	if (module.hot.data) {
		require(${loaderUtils.stringifyRequest(this, path.join(__dirname, "hotModuleReplacement.js"))})("${publicPath}", "%%extracted-file%%");
	}
}`;

Also, shouldn't compiling without ever moving css into js chunks be faster? Maybe this is part of the masterplan and needs to happen in core somewhere, but if CSS never makes its way into javascript modules/chunks, and is put off to the side early on, then we're talking not just easier for developers, but faster and maybe easier in implementation.

Customizingimport() may be one of those situations where we can work smarter, not harder.

});
Copy link
Member

Choose a reason for hiding this comment

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

\n

chunk.addModule(newModule);
});
}
});
});

compilation.plugin('additional-assets', (callback) => {
extractedChunks.forEach((extractedChunk) => {
if (extractedChunk.getNumberOfModules()) {
Expand Down
0 schema/loader.json → src/schema/loader.json
View file
Open in desktop
File renamed without changes.
0 schema/plugin.json → src/schema/plugin.json
View file
Open in desktop
File renamed without changes.
226 changes: 226 additions & 0 deletions test/__snapshots__/webpack-integration.test.js.snap
View file
Open in desktop
Expand Up @@ -83,6 +83,232 @@ c
"
`;

exports[`Webpack Integration Tests multiple-entries-all-async 1`] = `
"webpackJsonp([0],{

/***/ 1:
/***/ (function(module, exports, __webpack_require__) {

__webpack_require__(7);

modules.export = function() {
return 'Route Homepage';
};


/***/ }),

/***/ 7:
/***/ (function(module, exports) {

// removed by extract-text-webpack-plugin

/***/ })

});"
`;

exports[`Webpack Integration Tests multiple-entries-all-async 2`] = `
"webpackJsonp([1],{

/***/ 0:
/***/ (function(module, exports, __webpack_require__) {

__webpack_require__(6);

modules.export = function() {
return 'Route Contact';
};


/***/ }),

/***/ 6:
/***/ (function(module, exports) {

// removed by extract-text-webpack-plugin

/***/ })

});"
`;

exports[`Webpack Integration Tests multiple-entries-all-async 3`] = `
"body {
background: red;
}
.contact {
color: black;
}
.homepage {
color: black;
}
"
`;

exports[`Webpack Integration Tests multiple-entries-all-async 4`] = `
"body {
background: red;
}
.contact {
color: black;
}
.homepage {
color: black;
}
"
`;

exports[`Webpack Integration Tests multiple-entries-async 1`] = `
"webpackJsonp([0],{

/***/ 12:
/***/ (function(module, exports, __webpack_require__) {

// style-loader: Adds some css to the DOM by adding a <style> tag

// load the styles
var content = __webpack_require__(13);
if(typeof content === 'string') content = [[module.i, content, '']];
// Prepare cssTransformation
var transform;

var options = {}
options.transform = transform
// add the styles to the DOM
var update = __webpack_require__(1)(content, options);
if(content.locals) module.exports = content.locals;
// Hot Module Replacement
if(false) {
// When the styles change, update the <style> tags
if(!content.locals) {
module.hot.accept(\\"!!../../../../../node_modules/css-loader/index.js??ref--0-2!./styles.css\\", function() {
var newContent = require(\\"!!../../../../../node_modules/css-loader/index.js??ref--0-2!./styles.css\\");
if(typeof newContent === 'string') newContent = [[module.id, newContent, '']];
update(newContent);
});
}
// When the module is disposed, remove the <style> tags
module.hot.dispose(function() { update(); });
}

/***/ }),

/***/ 13:
/***/ (function(module, exports, __webpack_require__) {

exports = module.exports = __webpack_require__(0)(false);
// imports


// module
exports.push([module.i, \\".homepage {\\\\n\\\\tcolor: yellow;\\\\n}\\\\n\\", \\"\\"]);

// exports


/***/ }),

/***/ 3:
/***/ (function(module, exports, __webpack_require__) {

__webpack_require__(12);

modules.export = function() {
return 'Route Homepage';
};


/***/ })

});"
`;

exports[`Webpack Integration Tests multiple-entries-async 2`] = `
"webpackJsonp([1],{

/***/ 10:
/***/ (function(module, exports, __webpack_require__) {

// style-loader: Adds some css to the DOM by adding a <style> tag

// load the styles
var content = __webpack_require__(11);
if(typeof content === 'string') content = [[module.i, content, '']];
// Prepare cssTransformation
var transform;

var options = {}
options.transform = transform
// add the styles to the DOM
var update = __webpack_require__(1)(content, options);
if(content.locals) module.exports = content.locals;
// Hot Module Replacement
if(false) {
// When the styles change, update the <style> tags
if(!content.locals) {
module.hot.accept(\\"!!../../../../../node_modules/css-loader/index.js??ref--0-2!./styles.css\\", function() {
var newContent = require(\\"!!../../../../../node_modules/css-loader/index.js??ref--0-2!./styles.css\\");
if(typeof newContent === 'string') newContent = [[module.id, newContent, '']];
update(newContent);
});
}
// When the module is disposed, remove the <style> tags
module.hot.dispose(function() { update(); });
}

/***/ }),

/***/ 11:
/***/ (function(module, exports, __webpack_require__) {

exports = module.exports = __webpack_require__(0)(false);
// imports


// module
exports.push([module.i, \\".contact {\\\\n\\\\tcolor: black;\\\\n}\\\\n\\", \\"\\"]);

// exports


/***/ }),

/***/ 2:
/***/ (function(module, exports, __webpack_require__) {

__webpack_require__(10);

modules.export = function() {
return 'Route Contact';
};


/***/ })

});"
`;

exports[`Webpack Integration Tests multiple-entries-async 3`] = `
"body {
background: red;
}
.contact {
color: black;
}
"
`;

exports[`Webpack Integration Tests multiple-entries-async 4`] = `
"body {
background: red;
}
.homepage {
color: yellow;
}
"
`;

exports[`Webpack Integration Tests multiple-entries-filename 1`] = `
"a
b
Expand Down
3 changes: 3 additions & 0 deletions test/cases/multiple-entries-all-async/default-styles.css
View file
Open in desktop
@@ -0,0 +1,3 @@
body {
background: red;
}
2 changes: 2 additions & 0 deletions test/cases/multiple-entries-all-async/entries/contact.js
View file
Open in desktop
@@ -0,0 +1,2 @@
require('../router');
require('../routes/contact');
2 changes: 2 additions & 0 deletions test/cases/multiple-entries-all-async/entries/homepage.js
View file
Open in desktop
@@ -0,0 +1,2 @@
require('../router');
require('../routes/homepage');