Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4099 ES6 Refactor lib/HotModuleReplacementPlugin #5081

Merged
merged 5 commits into from Jul 6, 2017
Merged

4099 ES6 Refactor lib/HotModuleReplacementPlugin #5081

merged 5 commits into from Jul 6, 2017

Conversation

KTruong008
Copy link
Contributor

What kind of change does this PR introduce?

ES6 Refactor of HotModuleReplacementPlugin

Did you add tests for your changes?

Tests were already written.

Summary

Issue:
#4099

Other Information

Still working on this PR but I'm a bit stuck and looking for any advice. I managed to fix a lot of the broken tests as a result of the refactor, except for the last few that all seem to stem from the same problem.

All of the broken tests say something along the lines of "TestCases all-combined chunks .... should compile", along with red text underneath:

Unexpected token: keyword (const) [bundle.js:218,16]

I tried changing everything from const to let just to see how it would go, but then it had issues in the same test file with 'chunkId'.

Does anyone know how to run this specific test (TestCases.test.js all-combined) instead of having to run the whole test suite, so I can better debug it?

I tried $(npm bin)/mocha --grep TestCases, but it takes longer to run that single command than it does running the whole test suite because it keeps pausing and giving me weird messages like this:

HEAD 2881ms ± 306ms [2742ms; 3020ms]
․ v3.0.0-rc.2 2973ms ± 338ms [2820ms; 3127ms]
․․ HEAD 1462ms ± 90ms [1431ms; 1493ms]

.replace(/\$require\$/g, this.requireFn)
.replace(/\$hash\$/g, JSON.stringify(hash))
.replace(/\$requestTimeout\$/g, requestTimeout)
.replace(/\/\*foreachInstalledChunks\*\//g, chunk.chunks.length > 0 ? "for(const chunkId in installedChunks)" : `const chunkId = ${JSON.stringify(chunk.id)};`)
Copy link
Member

Choose a reason for hiding this comment

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

const chunkId -> var chunkId

This is code which is in the generated bundle. The generated code should not contain higher ES.

@sokra sokra reopened this Jun 18, 2017
@webpack webpack deleted a comment from webpack-bot Jun 18, 2017
const buf = [source];
buf.push("");
buf.push("// __webpack_hash__");
buf.push(`${this.requireFn}.h = () => hotCurrentHash`);
Copy link
Member

Choose a reason for hiding this comment

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

don't refactor the generated code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stopped refactoring the generated code on line 156

@webpack-bot
Copy link
Contributor

@KTruong888 Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

@sokra
Copy link
Member

sokra commented Jul 6, 2017

  2) TestCases all-combined chunks context should compile:
     Error: Errors while compiling:

bundle.js from UglifyJs
Unexpected token: punc ()) [bundle.js:726,35]

It looks like there is more generated code in ES6 format.

This is how to find it:

  • run the test locally: mocha -g "all-combined chunks context"
  • open the file test/js/all-combined/chunks/context/bundle.js
  • check the line from the error bundle.js:726,35 -> line 726, column 35
  • find the origin of the generated code in the HotModuleReplacementPlugin (or paste it here)
  • change back to ES3 code.

@webpack-bot
Copy link
Contributor

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@webpack-bot
Copy link
Contributor

It looks like this Pull Request doesn't include enough test cases (based on Code Coverage analysis of the PR diff).

A PR need to be covered by tests if you add a new feature (we want to make sure that your feature is working) or if you fix a bug (we want to make sure that we don't run into a regression in future).

@KTruong888 Please check if this is appliable to your PR and if you can add more test cases.

Read the test readme for details how to write test cases.

@sokra
Copy link
Member

sokra commented Jul 6, 2017

  1. TestCases all-combined chunks context should compile:
    Error: Errors while compiling:

bundle.js from UglifyJs
Unexpected token: punc ()) [bundle.js:726,35]
It looks like there is more generated code in ES6 format.

This is how to find it:

run the test locally: mocha -g "all-combined chunks context"
open the file test/js/all-combined/chunks/context/bundle.js
check the line from the error bundle.js:726,35 -> line 726, column 35
find the origin of the generated code in the HotModuleReplacementPlugin (or paste it here)
change back to ES3 code.

Sorry, I was wrong. It's all fine now.

@sokra sokra merged commit 2eccb19 into webpack:master Jul 6, 2017
@sokra
Copy link
Member

sokra commented Jul 6, 2017

Thanks

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

Successfully merging this pull request may close these issues.

None yet

3 participants