Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

fix(index): handle module extraction correctly (require.ensure) #687

Merged
merged 3 commits into from
Mar 20, 2018
Merged

fix(index): handle module extraction correctly (require.ensure) #687

merged 3 commits into from
Mar 20, 2018

Conversation

lvming6816077
Copy link
Contributor

@lvming6816077 lvming6816077 commented Dec 17, 2017 •
edited by michael-ciniawsky
Loading

@jsf-clabot
Copy link

jsf-clabot commented Dec 17, 2017 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@lvming6816077 lvming6816077 changed the title fix fix webpack require.ensure issues Dec 18, 2017
alexander-akait
alexander-akait suggested changes Dec 18, 2017
View reviewed changes
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need tests

joshwiens
joshwiens reviewed Dec 18, 2017
View reviewed changes
Copy link
Member

@joshwiens joshwiens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLA needs to be signed.

As @evilebottnawi suggested, this needs a failing & passing test.

Also, a bit of information about your Webpack environment would be helpful.

i.e. extract-text + webpack versions

@lvming6816077
add test
f86a134 
add test
@lvming6816077
Copy link
Contributor Author

lvming6816077 commented Dec 18, 2017 •
edited
Loading

I have commited test case and run test.
extract-text-webpack-plugin v3.0.2
webpack v3.9.1

TENNYLV-MB0:extract-text-webpack-plugin lvming$ npm test

 PASS  test/define-fallback.test.js
 PASS  test/define-loader.test.js
 PASS  test/schema-validation.test.js
 PASS  test/extract-text-plugin.test.js

 RUNS  test/webpack-integration.test.js
 PASS  test/webpack-integration.test.js

Test Suites: 5 passed, 5 total
Tests:       39 passed, 39 total
Snapshots:   26 passed, 26 total
Time:        5.042s
Ran all test suites.

alexander-akait
alexander-akait approved these changes Feb 1, 2018
View reviewed changes
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work!
@michael-ciniawsky Do you know why some CI failed?

@michael-ciniawsky michael-ciniawsky changed the title fix webpack require.ensure issues fix(index): require.ensure Feb 2, 2018
@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Feb 2, 2018 •
edited
Loading

@evilebottnawi There was a switch made to CircleCI for next and it seems Travis isn't enabled for master anymore, which needs to be fixed somehow...

@alexander-akait
Copy link
Member

@michael-ciniawsky locally test passed, seems we can approved and shipped

michael-ciniawsky
michael-ciniawsky suggested changes Feb 28, 2018
View reviewed changes
src/index.js Outdated
@@ -143,7 +143,7 @@ class ExtractTextPlugin {
});
async.forEach(chunks, (chunk, callback) => { // eslint-disable-line no-shadow
const extractedChunk = extractedChunks[chunks.indexOf(chunk)];
const shouldExtract = !!(options.allChunks || isInitialOrHasNoParents(chunk));
const shouldExtract = !!(options.allChunks);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary parentheses

src/index.js Outdated
if (shouldExtract && !wasExtracted) {
module[`${NS}/extract`] = shouldExtract; // eslint-disable-line no-path-concat

const cks = module.chunks;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cks => chunks or just use module.chunks directly

src/index.js Outdated
// check every module's chunks.parents() to decide extract or not
for (let i = 0; i < cks.length; i++) {
if (!isInitialOrHasNoParents(cks[i]) && !module.noExtracted) {
module.noExtracted = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

module.noExtracted => module.extracted and use a {Boolean} e.g

if (...) {
  module.extracted = true;

  break;
}

@michael-ciniawsky michael-ciniawsky added this to the 3.0.3 milestone Feb 28, 2018
@lvming6816077
Copy link
Contributor Author

@michael-ciniawsky thanks for your suggestion，I have committed the code refine，please check！

michael-ciniawsky
michael-ciniawsky approved these changes Mar 19, 2018
View reviewed changes
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lvming6816077 Thx

@alexander-akait
Copy link
Member

@michael-ciniawsky ready for merge 👍

@michael-ciniawsky
Copy link
Member

@lvming6816077 I will try to get this out as soon as possible, but I need to do some chores beforehand and merge other outstanding PR's aswell (ETA ~1-2 days)

@michael-ciniawsky michael-ciniawsky changed the title fix(index): require.ensure fix(index): handle module extraction correctly (require.ensure) Mar 20, 2018
@michael-ciniawsky michael-ciniawsky merged commit 219fe2d into webpack-contrib:master Mar 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joshwiens joshwiens joshwiens left review comments

@alexander-akait alexander-akait alexander-akait approved these changes

@michael-ciniawsky michael-ciniawsky michael-ciniawsky approved these changes

Assignees
No one assigned
Labels
semver: Patch status: Approved type: Bug
Projects
None yet
Milestone
3.0.3
Development

Successfully merging this pull request may close these issues.
5 participants
@lvming6816077 @jsf-clabot @michael-ciniawsky @alexander-akait @joshwiens