-
Notifications
You must be signed in to change notification settings - Fork 3
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
Version update - most imporantly Webpack 4 #113
Conversation
We need to update 'package.json' file with the actual current versions. |
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.
We should also test this branch against a few sites that are already in production, both watch
and build
@@ -81,7 +81,7 @@ module.exports = function CssLoader(source, map) { | |||
|
|||
function executeModuleAt(url) { | |||
return source => { | |||
const completeSource = `const __webpack_public_path__ = '${self.options.output.publicPath || '/'}'\n` + source | |||
const completeSource = `const __webpack_public_path__ = '${self._compiler.options.output.publicPath || '/'}'\n` + source |
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.
module.exports = function websocketCommunicationPlugin() { | ||
|
||
return { | ||
apply: compiler => { | ||
// we should add this check to all hooks we create |
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.
Ok, do that
Used syncyarnlock to update
|
chunkAssets[chunk.name] = { | ||
filename, | ||
hasRuntime: chunk.hasRuntime(), | ||
parents: chunk.parents.map(c => c.name).filter(Boolean) | ||
isShared, | ||
...(!isShared && { dependencies: group.chunks.filter(x => x !== chunk).map(x => x.name) }) |
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.
Can not use this syntax for nodejs < 8.6
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.
Looks good! Still needs testing with some projects.
example/package.json
Outdated
"react-dom": "^16.2.0", | ||
"normalize.css": "^8.0.0", | ||
"react": "^16.3.1", | ||
"react-dom": "^16.3.1", |
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.
react
and react-dom
aren't necessary anymore - they were defined for [email protected]
library/README.md
Outdated
@@ -2,6 +2,7 @@ | |||
|
|||
Breaking changes: | |||
|
|||
- v0.0.?? - `chunk-manifest.json` changed |
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.
don't forget this
"helmet": "^3.9.0", | ||
"express": "^4.16.3", | ||
"file-loader": "^1.1.11", | ||
"find-yarn-workspace-root": "^1.1.0", |
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.
🎉
@@ -25,7 +30,8 @@ const fragmentResolverPlugin = require('../webpack-resolver-plugins/fragment-res | |||
const CaseSensitivePathsPlugin = require('case-sensitive-paths-webpack-plugin') | |||
const ExtendedAPIPlugin = require('webpack/lib/ExtendedAPIPlugin') | |||
const ProgressBarPlugin = require('progress-bar-webpack-plugin') | |||
const findYarnWorkspaceRoot = require('find-yarn-workspace-root') | |||
const TimeFixPlugin = require('time-fix-plugin') // https://github.com/webpack/watchpack/issues/25 |
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.
Will this fix the weird build loops which occasionally happened?
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.
Most of them, yes
@@ -25,7 +30,8 @@ const fragmentResolverPlugin = require('../webpack-resolver-plugins/fragment-res | |||
const CaseSensitivePathsPlugin = require('case-sensitive-paths-webpack-plugin') | |||
const ExtendedAPIPlugin = require('webpack/lib/ExtendedAPIPlugin') | |||
const ProgressBarPlugin = require('progress-bar-webpack-plugin') | |||
const findYarnWorkspaceRoot = require('find-yarn-workspace-root') | |||
const TimeFixPlugin = require('time-fix-plugin') // https://github.com/webpack/watchpack/issues/25 | |||
const UglifyJsPlugin = require('uglifyjs-webpack-plugin') |
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.
🎉
}) | ||
}) | ||
|
||
// tell the web compiler to compile, emit the assets and notify the appropriate plugins | ||
compiler.plugin('make-additional-entries', (compilation, createEntries, done) => { | ||
// Note, we can not use `make` because it's parralel | ||
compiler.hooks.makeAdditionalEntries.tapAsync(p, (compilation, _, done) => { |
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.
how do we name the third argument, callback
or done
?
compilation.dependencyFactories.set(ConstDependency, new NullFactory()) | ||
compilation.dependencyTemplates.set(ConstDependency, new ConstDependency.Template()) | ||
compilation.mainTemplate.plugin('require-extensions', function(source, chunk, hash) { | ||
compilation.mainTemplate.hooks.requireExtensions.tap(p, function(source, chunk, hash) { |
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.
arrow function is fine
}) | ||
compilation.mainTemplate.hooks.globalHash.tap(p, () => true) | ||
normalModuleFactory.hooks.parser.for('javascript/auto').tap(p, addParserHooks) | ||
normalModuleFactory.hooks.parser.for('javascript/dynamic').tap(p, addParserHooks) |
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.
no idea what's happening here with the auto/dynamic stuff?
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.
Check here for an explanation of these: https://medium.com/webpack/webpack-4-released-today-6cdb994702d4#b6a9
It essentially requires us to define in which types of files this hook should trigger.
}) | ||
compilation.mainTemplate.hooks.globalHash.tap(p, () => true) | ||
normalModuleFactory.hooks.parser.for('javascript/auto').tap(p, addParserHooks) | ||
normalModuleFactory.hooks.parser.for('javascript/dynamic').tap(p, addParserHooks) |
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.
I notice there's duplicate code in merge-css-plugin
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.
What is the duplication?
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.
compilation.dependencyFactories.set(ConstDependency, new NullFactory())
compilation.dependencyTemplates.set(ConstDependency, new ConstDependency.Template())
compilation.mainTemplate.hooks.requireExtensions.tap(p, function(source, chunk, hash) {
// ...
}
compilation.mainTemplate.hooks.globalHash.tap(p, () => true)
normalModuleFactory.hooks.parser.for('javascript/auto').tap(p, addParserHooks)
normalModuleFactory.hooks.parser.for('javascript/dynamic').tap(p, addParserHooks)
Might be just fine like this
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.
Ahh, yeah, the code looks very similar. It even is similar, but I am unsure if I want to extract it. I'll experiment with it.
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.
I changed it, despite the amount of arguments required I think it's a good abstraction. It also showed a few minor flaws so it already paid off (or proved that evaluateTypeOf
was not used).
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.
Haha, check what I found (committed at Oct 1, 2017): https://github.com/kaliberjs/build/blob/aec93e775f305362b15acf864741adb49e6ef31d/library/lib/webpack-utils.js
The code is very similar, haha: https://github.com/kaliberjs/build/blob/89b75cf5ade159f4dfb4cc8ab9ff4ad247a9db1a/library/lib/webpack-utils.js
}) | ||
compilation.mainTemplate.hooks.globalHash.tap(p, () => true) | ||
normalModuleFactory.hooks.parser.for('javascript/auto').tap(p, addParserHooks) | ||
normalModuleFactory.hooks.parser.for('javascript/dynamic').tap(p, addParserHooks) |
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.
more duplicate code
Ok, I think the only thing we need to do for this release is test 😄 |
Conflicts: package.json yarn.lock
`*.entry.css` classnames are no longer hashed
#110