-
Notifications
You must be signed in to change notification settings - Fork 513
fix: split async CSS correctly #546
Changes from 25 commits
77daf83
92e4349
cdfccb4
3e78452
4880afb
5df5d09
b202f4a
292e217
28171b2
1730d46
5d0c28f
715b1dd
1ef755a
84a0328
b33e006
d4a0c23
10721f5
1175e63
fb4eb9b
400ed49
143760a
163191a
2ba04eb
3ef2916
8403a27
3aed4df
f1a8ce2
edd39a1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ import { | |
mergeOptions, | ||
isString, | ||
isFunction, | ||
cloneModule, | ||
} from './lib/helpers'; | ||
|
||
const NS = path.dirname(fs.realpathSync(__filename)); | ||
|
@@ -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'); | ||
} | ||
this.filename = options.filename; | ||
this.id = options.id != null ? options.id : ++nextId; | ||
|
@@ -88,7 +89,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)'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should also be reverted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I run the tests with
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because you moved the |
||
} | ||
let loader = options.use; | ||
let before = options.fallback || []; | ||
|
@@ -126,8 +127,10 @@ class ExtractTextPlugin { | |
const filename = this.filename; | ||
const id = this.id; | ||
let extractedChunks; | ||
let toRemoveModules; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
compilation.plugin('optimize-tree', (chunks, modules, callback) => { | ||
extractedChunks = chunks.map(() => new Chunk()); | ||
toRemoveModules = []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
chunks.forEach((chunk, i) => { | ||
const extractedChunk = extractedChunks[i]; | ||
extractedChunk.index = i; | ||
|
@@ -145,28 +148,41 @@ 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 => c), (module, callback) => { // eslint-disable-line no-shadow, arrow-body-style | ||
let meta = module[NS]; | ||
if (meta && (!meta.options.id || meta.options.id === id)) { | ||
const wasExtracted = Array.isArray(meta.content); | ||
// A stricter `shouldExtract !== wasExtracted` check to guard against cases where a previously extracted | ||
// module would be extracted twice. Happens when a module is a dependency of an initial and a non-initial | ||
// chunk. See issue #604 | ||
if (shouldExtract && !wasExtracted) { | ||
module[`${NS}/extract`] = shouldExtract; // eslint-disable-line no-path-concat | ||
compilation.rebuildModule(module, (err) => { | ||
const newModule = cloneModule(module); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
newModule[`${NS}/extract`] = shouldExtract; // eslint-disable-line no-path-concat | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
compilation.buildModule(newModule, false, newModule, null, (err) => { | ||
if (err) { | ||
compilation.errors.push(err); | ||
return callback(); | ||
} | ||
meta = module[NS]; | ||
meta = newModule[NS]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
const identifier = module.identifier(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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 { | ||
|
@@ -194,6 +210,29 @@ class ExtractTextPlugin { | |
callback(); | ||
}); | ||
}); | ||
|
||
compilation.plugin('optimize-module-ids', (modules) => { | ||
modules.forEach((module) => { | ||
const data = toRemoveModules[module.identifier()]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if (data) { | ||
const oldModuleId = module.id; | ||
const newModule = cloneModule(module); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
newModule.id = oldModuleId; | ||
newModule._source = data.module._source; // eslint-disable-line no-underscore-dangle | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
data.chunks.forEach((chunk) => { | ||
chunk.removeModule(data.moduleToRemove); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
const deps = data.moduleToRemove.dependencies; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
deps.forEach((d) => { | ||
if (d.module && d.module.loaders.length > 0) { | ||
chunk.removeModule(d.module); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. About this, I realized that There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: I.e. 2 requests, rather than what I was doing before with 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 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 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:
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. Customizing |
||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
chunk.addModule(newModule); | ||
}); | ||
} | ||
}); | ||
}); | ||
|
||
compilation.plugin('additional-assets', (callback) => { | ||
extractedChunks.forEach((extractedChunk) => { | ||
if (extractedChunk.getNumberOfModules()) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
body { | ||
background: red; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
require('../router'); | ||
require('../routes/contact'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
require('../router'); | ||
require('../routes/homepage'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 thebuild
script inpackage.json
please.- schema src ||– index.js ||– Plugin.json ||- loader.js ||- loader.json | |-package.json