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

fix(index): don't crash with dynamic import() #728

Merged
merged 2 commits into from
Feb 28, 2018
Merged

fix(index): don't crash with dynamic import() #728

merged 2 commits into from
Feb 28, 2018

Conversation

Jessidhia
Copy link
Contributor

@Jessidhia Jessidhia commented Feb 27, 2018 •
edited by michael-ciniawsky
Loading

Issues

as well as works around an issue where javascript source code of concat modules is being included in extracted css.

Notable Changes

This also works around an issue where meta.content can have more than one entry, which I could not figure out how to reproduce for the unit tests, but which was happening in a codebase I am trying to migrate to webpack@4. I don't know if the result is 100% correct, but it compiles, is no longer missing huge amounts of CSS that was definitely supposed to be there, and on quick inspection doesn't seem to have visual artifacts on my page.

This also fixes the commitlint config because it didn't want to let me commit otherwise 🤷‍♀️

Jessidhia
Jessidhia commented Feb 27, 2018
View reviewed changes
src/index.js Outdated
@@ -218,7 +233,9 @@ class ExtractTextPlugin {
// chunk.sortModules();

async.forEach(
Array.from(chunk.modulesIterable),
Array.from(chunk.modulesIterable).sort(
(a, b) => a.index - b.index
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes the chunk-modules-css-wrong-order test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this use .index2 instead?

Copy link
Member

Choose a reason for hiding this comment

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

index2 is the ESM evaluation order. index is the CJS evaluation order @sokra

css-loader@latest returns a CJS module, so index should be fine for now. Please add a comment to check/switch to index2 in case this plugin will see ESM input in the future 😛

Copy link
Contributor Author

@Jessidhia Jessidhia Feb 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

That's actually not up to css-loader, but up to the ETWP pitching loader's result. The pitch result is a commonjs module, so even if css-loader is converted, it won't be able to be picked by concatenated modules because it'll be required via commonjs.

@Jessidhia
Copy link
Contributor Author

I have no idea what is broken in CI but it's not our code 🤔

@alexander-akait alexander-akait added this to the 3.0.3 milestone Feb 27, 2018
@alexander-akait
Copy link
Member

@Kovensky very strange, seems problem in worker-farm in uglify plugin 😕

@Jessidhia
Copy link
Contributor Author

Jessidhia commented Feb 27, 2018 •
edited
Loading

I suspect a conflict because of jsdom, which jest loads by default. jsdom might be injecting its own setTimeout which doesn't return the same value as node's setTimeout. (Browser setTimeout returns number; node setTimeout returns an object)

@alexander-akait alexander-akait modified the milestones: 3.0.3, 4.0.0 Feb 27, 2018
@quantizor
Copy link

@Kovensky you should be able to use this comment to disable JSDOM for a test file:

/**
 * @jest-environment node
 */

michael-ciniawsky
michael-ciniawsky approved these changes Feb 27, 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.

Maybe better to wait for webpack/webpack#6597

src/index.js Outdated
@@ -218,7 +233,9 @@ class ExtractTextPlugin {
// chunk.sortModules();

async.forEach(
Array.from(chunk.modulesIterable),
Array.from(chunk.modulesIterable).sort(
(a, b) => a.index - b.index
Copy link
Member

Choose a reason for hiding this comment

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

index2 is the ESM evaluation order. index is the CJS evaluation order @sokra

css-loader@latest returns a CJS module, so index should be fine for now. Please add a comment to check/switch to index2 in case this plugin will see ESM input in the future 😛

src/index.js
const source = new ConcatSource();

for (const chunkModule of chunk.modulesIterable) {
let moduleSource = chunkModule.source();
let moduleSource = chunkModule.source(
Copy link
Member

Choose a reason for hiding this comment

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

webpack/webpack#6597

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That workaround breaks for other types of Module; it's best to just treat these 2 arguments as part of the Module API now.

commitlint.config.js Outdated
@@ -12,7 +12,7 @@ const Configuration = {
'subject-full-stop': [2, 'never', '.'],
'type-case': [2, 'always', 'lower-case'],
'type-empty': [2, 'never'],
'type-enum': [
'type-enum': [2, 'always', [
Copy link
Member

@michael-ciniawsky michael-ciniawsky Feb 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Revert please, if it is a bug then it needs to be fixed in webpack-defaults instead. Not sure actually, seeing this commit messages :D :P https://github.com/webpack-contrib/extract-text-webpack-plugin/pull/728/commits. Use git commit -m '...' --no-verify in the meantime, they will be squashed anyways :)

@michael-ciniawsky michael-ciniawsky changed the title Fix crash when used with dynamic imports in webpack@4 fix(index): crash when used with dynamic import() Feb 27, 2018
chore: Correct jest's testEnvironment
04893fb 
ETWP is not supposed to run in a browser
@Jessidhia
Copy link
Contributor Author

@probablyup that problem wasn't with one file but with all files; since ETWP is supposed to run on node instead of in a browser the default testEnvironment was wrong to begin with.

@michael-ciniawsky michael-ciniawsky merged commit 348b46b into webpack-contrib:next Feb 28, 2018
@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Feb 28, 2018 •
edited
Loading

Released in v4.0.0-beta.0 🎉

ℹ️ Last release under extract-text-webpack-plugin@next (if nothing urgent pops up :))

@michael-ciniawsky michael-ciniawsky changed the title fix(index): crash when used with dynamic import() fix(index): don't crash with dynamic import() Feb 28, 2018
@clarkdo clarkdo mentioned this pull request Mar 1, 2018
Closed
joshwiens pushed a commit that referenced this pull request Apr 2, 2018
@joshwiens
fix(index): don't crash with dynamic import() (#728)
602a5b0
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

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

@ooflorent ooflorent Awaiting requested review from ooflorent

@sokra sokra Awaiting requested review from sokra

@alexander-akait alexander-akait Awaiting requested review from alexander-akait

@joshwiens joshwiens Awaiting requested review from joshwiens

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@Jessidhia @alexander-akait @quantizor @michael-ciniawsky