Dynamic entry support #3634

Merged
merged 11 commits into from Jan 9, 2017

Projects

None yet

4 participants

@nkzawa
Contributor
nkzawa commented Dec 29, 2016 edited

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 entry option during watch mode.
Related: #3354, #2874

Does this PR introduce a breaking change?
No.

Other information

nkzawa added some commits Dec 29, 2016
@nkzawa nkzawa enable to set a function to entry option aa68c61
@nkzawa nkzawa add tests for entry function 55cb174
@nkzawa nkzawa fix style
b2661a3
@TheLarkInn TheLarkInn requested a review from sokra Dec 29, 2016
lib/MultiModule.js
- return "multi " + this.name;
+ return "multi " + this.dependencies.map(function(d) {
+ return path.resolve(this.context, d.request);
+ }.bind(this)).join("!");
@sokra
sokra Dec 30, 2016 Member

Why was this changed?

@nkzawa
nkzawa Dec 30, 2016 Contributor

This is because an entry with a certain name might has different dependencies when the entry function was called multiple times.

@sokra
sokra Dec 30, 2016 Member

Ok makes sense. Could you update readableIdentifier too and completely remove this.name from the MultiModule.

@nkzawa
nkzawa Dec 30, 2016 Contributor

How should I update readableIdentifier ? I thought it's for human then this.name makes sense ?

@sokra
sokra Jan 2, 2017 Member

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.

@nkzawa nkzawa referenced this pull request in zeit/next.js Jan 2, 2017
Open

Lazy compilation during development #608

@nkzawa
Contributor
nkzawa commented Jan 2, 2017

@sokra @TheLarkInn Let me know if there are anything else I should work on.

@sokra

identifier and readableIdentifier

@nkzawa nkzawa fix identifier and readableIdentifier of MultiModule
00b60e3
@nkzawa
Contributor
nkzawa commented Jan 2, 2017

@sokra Fixed identifier and readableIdentifier. Please check.

lib/MultiModule.js
- return path.resolve(this.context, d.request);
- }.bind(this)).join("!");
+ return d.request;
+ }.bind(this)).join(" ");
@chicoxyzzy
chicoxyzzy Jan 2, 2017 Contributor

this should use second parameter of map or even better you can use an arrow function

lib/MultiModule.js
+MultiModule.prototype.readableIdentifier = function(requestShortener) {
+ return "multi " + this.dependencies.map(function(d) {
+ return requestShortener.shorten(d.request);
+ }.bind(this)).join(" ");
@chicoxyzzy
chicoxyzzy Jan 2, 2017 edited Contributor

same here and other places

@nkzawa nkzawa use arrow functions
0f64911
lib/MultiEntryPlugin.js
+ var dep = new SingleEntryDependency(e);
+ dep.loc = name + ":" + (100000 + idx);
+ return dep;
+ }, this), name);
@chicoxyzzy
chicoxyzzy Jan 3, 2017 Contributor

use arrow function here please

lib/MultiEntryPlugin.js
- return dep;
- }, this), this.name), this.name, callback);
+ var dep = MultiEntryPlugin.createDependency(this.entries, this.name);
+ compilation.addEntry(this.context, dep, this.name, callback);
}.bind(this));
@chicoxyzzy
chicoxyzzy Jan 3, 2017 Contributor

use compiler instead of this, add arrow function and remove bind

@nkzawa
nkzawa Jan 3, 2017 Contributor

I think fixing styles is not the scope of this PR. I will fix what I added.

@chicoxyzzy
chicoxyzzy Jan 3, 2017 Contributor

fair point

lib/SingleEntryPlugin.js
SingleEntryPlugin.prototype.apply = function(compiler) {
compiler.plugin("compilation", function(compilation, params) {
var normalModuleFactory = params.normalModuleFactory;
compilation.dependencyFactories.set(SingleEntryDependency, normalModuleFactory);
});
compiler.plugin("make", function(compilation, callback) {
- var dep = new SingleEntryDependency(this.entry);
- dep.loc = this.name;
+ var dep = SingleEntryPlugin.createDependency(this.entry, this.name);
compilation.addEntry(this.context, dep, this.name, callback);
}.bind(this));
@chicoxyzzy
chicoxyzzy Jan 3, 2017 Contributor

same here

@nkzawa nkzawa use arrow function
890d7c4
@sokra

Looks good.

Take a look at the CI output. You need to update some Validation tests because of the schema update.

lib/DynamicEntryPlugin.js
+
+DynamicEntryPlugin.createDependency = function(entry, name) {
+ if(Array.isArray(entry))
+ return new MultiEntryPlugin.createDependency(entry, name);
@sokra
sokra Jan 4, 2017 Member

The new is incorrect here.

lib/DynamicEntryPlugin.js
+
+DynamicEntryPlugin.createDependency = function(entry, name) {
+ if(Array.isArray(entry))
+ return new MultiEntryPlugin.createDependency(entry, name);
@sokra
sokra Jan 4, 2017 Member

new is incorrect here.

nkzawa added some commits Jan 5, 2017
@nkzawa nkzawa remove incorrect new operators
ec7c3f5
@nkzawa nkzawa Merge branch 'master' into add/dynamic-entry
3ec021a
@nkzawa nkzawa fix Validation tests
3f5f864
@TheLarkInn
Member

Sync master and I think we are looking good.

@nkzawa nkzawa Merge branch 'master' into add/dynamic-entry
6e3eed3
@nkzawa
Contributor
nkzawa commented Jan 6, 2017

@TheLarkInn fixed conflicts.

@TheLarkInn
Member

Sorry we've been merging a lot lately. Mind syncing again and I'll.merge right away.

@nkzawa nkzawa Merge branch 'master' into add/dynamic-entry
47ee0d7
@nkzawa
Contributor
nkzawa commented Jan 9, 2017

@TheLarkInn done.

@TheLarkInn
Member

Thanks!!

@TheLarkInn TheLarkInn merged commit a22b00e into webpack:master Jan 9, 2017

7 checks passed

codecov/changes No unexpected coverage changes found.
Details
codecov/patch 98.07% of diff hit (target 93.32%)
Details
codecov/project 93.36% (+0.03%) compared to 42b95d9
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 93.62%
Details
licence/cla Contributor License Agreement is signed.
Details
@nkzawa nkzawa deleted the nkzawa:add/dynamic-entry branch Jan 9, 2017
@nkzawa nkzawa referenced this pull request in zeit/next.js Jan 12, 2017
Merged

Use dynamic entry feature of webpack #750

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment