deps: Add webpack 4 compatibility #33
Conversation
|
Test fails because webpack 4 is not compatible with Node.js 4. We can change the .travis.yml here, too, but I also created a PR for this #34 |
src/extractLoader.js
Outdated
| const publicPath = options.publicPath === undefined ? this.options.output.publicPath : options.publicPath; | ||
| const publicPath = options.publicPath || | ||
| (this.options && this.options.output && this.options.output.publicPath) || | ||
| (this._compilation && this._compilation.outputOptions && this._compilation.outputOptions.publicPath); |
There was a problem hiding this comment.
- I think we should check for
"publicPath" in optionshere because an empty string aspublicPathis a valid option - The check for
this.optionsis necessary with webpack 4, but could you add aTODOthat this should be removed with the next major release - I guess you've added the
this._compilationpart so that we can still derive thepublicPath. We should also remove that with the next major release sincethis._compilationis likely to be removed in the future.
There was a problem hiding this comment.
I've created an issue for that: #35
There was a problem hiding this comment.
- You're right. I'll change that.
- and 3. Remove access of
this.optionsandthis._compilation#35 is enough, isn't it?
test/extractLoader.test.js
Outdated
| stats.compilation.fileDependencies.forEach( | ||
| dependency => { | ||
| dependencies.push(dependency.slice(basePath.length)); | ||
| } |
There was a problem hiding this comment.
I think, this could be solved with Array.from:
const dependencies = Array.from(stats.compilation.fileDependencies)
.map(dependency => dependency.slice(basePath.length));What do you think?
There was a problem hiding this comment.
Yep, looks good. I'll change that 👍
|
I've addressed your remarks 👍 PTAL |
src/extractLoader.js
Outdated
| const publicPath = options.publicPath || | ||
| (this.options && this.options.output && this.options.output.publicPath) || | ||
| (this._compilation && this._compilation.outputOptions && this._compilation.outputOptions.publicPath); | ||
| const publicPath = options.publicPath || getPublicPath(this); |
There was a problem hiding this comment.
If options.publicPath was an empty string, the empty string should be used. Maybe change that line to:
const publicPath = "publicPath" in options ? options.publicPath : getPublicPath(this)Or maybe put that logic into getPublicPath as well?
There was a problem hiding this comment.
The idea behind getPublicPath() was a function that can be removed when we work on #35 .
That's why options was not included.
Should fix #32
Also should not break compatibility with previous webpack versions.