Conversation
lib/MultiModule.js
Outdated
| return "multi " + this.name; | ||
| return "multi " + this.dependencies.map(function(d) { | ||
| return path.resolve(this.context, d.request); | ||
| }.bind(this)).join("!"); |
There was a problem hiding this comment.
This is because an entry with a certain name might has different dependencies when the entry function was called multiple times.
There was a problem hiding this comment.
Ok makes sense. Could you update readableIdentifier too and completely remove this.name from the MultiModule.
There was a problem hiding this comment.
How should I update readableIdentifier ? I thought it's for human then this.name makes sense ?
There was a problem hiding this comment.
readableIdentifier should reflect identifier but in a human readable way. You can use requestShortener.shorten to generate a readable identifier from a request.
Other issue: identifier should only use d.request and not this.context. And don't join them by !. This doesn't make sense here (! is for loaders). Maybe just join them by | or space.
|
@sokra @TheLarkInn Let me know if there are anything else I should work on. |
sokra
left a comment
There was a problem hiding this comment.
identifier and readableIdentifier
|
@sokra Fixed |
lib/MultiModule.js
Outdated
| return path.resolve(this.context, d.request); | ||
| }.bind(this)).join("!"); | ||
| return d.request; | ||
| }.bind(this)).join(" "); |
There was a problem hiding this comment.
this should use second parameter of map or even better you can use an arrow function
lib/MultiModule.js
Outdated
| MultiModule.prototype.readableIdentifier = function(requestShortener) { | ||
| return "multi " + this.dependencies.map(function(d) { | ||
| return requestShortener.shorten(d.request); | ||
| }.bind(this)).join(" "); |
There was a problem hiding this comment.
same here and other places
lib/MultiEntryPlugin.js
Outdated
| var dep = new SingleEntryDependency(e); | ||
| dep.loc = name + ":" + (100000 + idx); | ||
| return dep; | ||
| }, this), name); |
There was a problem hiding this comment.
use arrow function here please
lib/MultiEntryPlugin.js
Outdated
| }, this), this.name), this.name, callback); | ||
| var dep = MultiEntryPlugin.createDependency(this.entries, this.name); | ||
| compilation.addEntry(this.context, dep, this.name, callback); | ||
| }.bind(this)); |
There was a problem hiding this comment.
use compiler instead of this, add arrow function and remove bind
There was a problem hiding this comment.
I think fixing styles is not the scope of this PR. I will fix what I added.
lib/SingleEntryPlugin.js
Outdated
| dep.loc = this.name; | ||
| var dep = SingleEntryPlugin.createDependency(this.entry, this.name); | ||
| compilation.addEntry(this.context, dep, this.name, callback); | ||
| }.bind(this)); |
sokra
left a comment
There was a problem hiding this comment.
Looks good.
Take a look at the CI output. You need to update some Validation tests because of the schema update.
lib/DynamicEntryPlugin.js
Outdated
|
|
||
| DynamicEntryPlugin.createDependency = function(entry, name) { | ||
| if(Array.isArray(entry)) | ||
| return new MultiEntryPlugin.createDependency(entry, name); |
lib/DynamicEntryPlugin.js
Outdated
|
|
||
| DynamicEntryPlugin.createDependency = function(entry, name) { | ||
| if(Array.isArray(entry)) | ||
| return new MultiEntryPlugin.createDependency(entry, name); |
|
Sync master and I think we are looking good. |
|
@TheLarkInn fixed conflicts. |
|
Sorry we've been merging a lot lately. Mind syncing again and I'll.merge right away. |
|
@TheLarkInn done. |
|
Thanks!! |
What kind of change does this PR introduce?
feature
Did you add tests for your changes?
yes
If relevant, link to documentation update:
Summary
Enable to change the value of the
entryoption during watch mode.Related: #3354, #2874
Does this PR introduce a breaking change?
No.
Other information