4099 ES6 Refactor lib/HotModuleReplacementPlugin by kylebuildsstuff · Pull Request #5081 · webpack/webpack

kylebuildsstuff · 2017-06-17T21:08:26Z

What kind of change does this PR introduce?

ES6 Refactor of HotModuleReplacementPlugin

Did you add tests for your changes?

Tests were already written.

Summary

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]
․

sokra · 2017-06-18T15:46:06Z

+					.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)};`)


const chunkId -> var chunkId

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

sokra · 2017-07-01T15:46:31Z

+				const buf = [source];
+				buf.push("");
+				buf.push("// __webpack_hash__");
+				buf.push(`${this.requireFn}.h = () => hotCurrentHash`);


don't refactor the generated code.

stopped refactoring the generated code on line 156

…n PR

webpack-bot · 2017-07-01T16:13:50Z

@KTruong888 Thanks for your update.

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

@sokra Please review the new changes.

…acy coding quality review

sokra · 2017-07-06T06:44:54Z

  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 · 2017-07-06T06:44:56Z

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

webpack-bot · 2017-07-06T06:44:56Z

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 · 2017-07-06T06:45:45Z

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 · 2017-07-06T06:47:14Z

Thanks

webpack-bot added PR: unreviewed PR: CI-not-ok labels Jun 17, 2017

kylebuildsstuff closed this Jun 18, 2017

sokra reviewed Jun 18, 2017

View reviewed changes

sokra reopened this Jun 18, 2017

webpack deleted a comment from webpack-bot Jun 18, 2017

sokra requested changes Jul 1, 2017

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Jul 1, 2017

Kyle Truong and others added 3 commits July 1, 2017 12:02

4099 ES6 Refactor lib/HotModuleReplacementPlugin

971bbb6

change to var in generated code

a6dea39

- Fix bug while refactoring HotModuleReplacementPlugin as suggested i…

c3d8fdf

…n PR

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Jul 1, 2017

Kyle Truong added 2 commits July 1, 2017 12:27

- Fix ES6 refactoring lib/HotModuleReplacementPlugin according to cod…

be13bf9

…acy coding quality review

- Undo previous commit because beautify error

5202091

webpack-bot added PR: CI-ok PR: tests-needed and removed PR: CI-not-ok labels Jul 6, 2017

sokra approved these changes Jul 6, 2017

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Jul 6, 2017

sokra merged commit 2eccb19 into webpack:master Jul 6, 2017

renovate-tests mentioned this pull request Jul 7, 2017

Update dependency webpack to v3 renovate-tests/discover-package-files#13

Open

This was referenced Jul 7, 2017

⬆️ webpack -> ~3.1.0 homezen/hz-test-helpers#54

Merged

⬆️ webpack @ ~3.1.0 homezen/redux-variants#164

Merged

renovate bot mentioned this pull request Jul 7, 2017

Update dependency webpack to version 3.x algolia/react-element-to-jsx-string#105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

4099 ES6 Refactor lib/HotModuleReplacementPlugin#5081

4099 ES6 Refactor lib/HotModuleReplacementPlugin#5081
sokra merged 5 commits intowebpack:masterfrom
kylebuildsstuff:4099_es6_refactor_lib_hot_module_replacement_plugin

kylebuildsstuff commented Jun 17, 2017

Uh oh!

sokra Jun 18, 2017

Uh oh!

sokra Jul 1, 2017

Uh oh!

kylebuildsstuff Jul 1, 2017

Uh oh!

webpack-bot commented Jul 1, 2017

Uh oh!

sokra commented Jul 6, 2017

Uh oh!

webpack-bot commented Jul 6, 2017

Uh oh!

webpack-bot commented Jul 6, 2017

Uh oh!

sokra commented Jul 6, 2017

Uh oh!

sokra commented Jul 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

kylebuildsstuff commented Jun 17, 2017

Uh oh!

sokra Jun 18, 2017

Choose a reason for hiding this comment

Uh oh!

sokra Jul 1, 2017

Choose a reason for hiding this comment

Uh oh!

kylebuildsstuff Jul 1, 2017

Choose a reason for hiding this comment

Uh oh!

webpack-bot commented Jul 1, 2017

Uh oh!

sokra commented Jul 6, 2017

Uh oh!

webpack-bot commented Jul 6, 2017

Uh oh!

webpack-bot commented Jul 6, 2017

Uh oh!

sokra commented Jul 6, 2017

Uh oh!

sokra commented Jul 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants