-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
4099 ES6 Refactor lib/HotModuleReplacementPlugin #5081
Conversation
lib/HotModuleReplacementPlugin.js
Outdated
.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)};`) |
There was a problem hiding this comment.
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.
lib/HotModuleReplacementPlugin.js
Outdated
const buf = [source]; | ||
buf.push(""); | ||
buf.push("// __webpack_hash__"); | ||
buf.push(`${this.requireFn}.h = () => hotCurrentHash`); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@KTruong888 Thanks for your update. I labeled the Pull Request so reviewers will review it again. @sokra Please review the new changes. |
It looks like there is more generated code in ES6 format. This is how to find it:
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
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. |
Sorry, I was wrong. It's all fine now. |
Thanks |
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]
․