-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix: ES5 client #2658
Fix: ES5 client #2658
Conversation
Codecov Report
@@ Coverage Diff @@
## v4 #2658 +/- ##
==========================================
+ Coverage 92.85% 92.86% +0.01%
==========================================
Files 37 39 +2
Lines 1301 1304 +3
Branches 348 349 +1
==========================================
+ Hits 1208 1211 +3
Misses 89 89
Partials 4 4
Continue to review full report at Codecov.
|
Looks like test instability again on Windows. |
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.
still LGTM 👍
filename: 'strip-ansi.js', | ||
}, | ||
}), | ||
]; |
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.
I am afraid, some modules can use new syntax (ES 6) and we can't catch it, for example in patch version
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.
@evilebottnawi If that happens when we are updating a dependency, it will be caught in CI here: https://github.com/webpack/webpack-dev-server/pull/2658/files#diff-24488ac612ba9666465a7a596040ac73R43
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.
But if we want full safety, we should do this: #2652. I don't like how that PR ended up because it has so many breaking changes in the API and seems a little risky
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.
Sound good to me. CI is enough for me.
26c9943
to
fe9b9b2
Compare
Ah, issue with lodash is making lint fail: https://www.npmjs.com/advisories/1523 |
b9757d0
to
f33be55
Compare
For Bugs and Features; did you add new tests?
Yes
Motivation / Use-Case
We need to make the client work with ES5 for webpack@4. This PR does some case-specific patches, rather than trying to fully solve the problem, since webpack@5 always has ES6 syntax in its bundles, unless the user uses babel-loader.
The
webpack/lib/logging/runtime
andstrip-ansi
modules use ES6, so they are placed in thetranspiled-modules
directory and have files that act as a proxy entry to them. Then, these entry files are turned into a bundle with babel-loader, so that the result is a module that works with ES5 and behaves the same way.Explanation for the ES5 check test: Since webpack takes all the JavaScript and puts it into a string in an
eval
function, a bundle that will not work with ES5 can still pass a basic ES5 check, since all this bad ES6 syntax is inside of a string. The best course of action for a test I think is to locate these strings in a bundle thateval
is being run on, then check that they parse with ES5.#1286 Actually was not fixed on v4 for the reason above, but should be fixed by this PR.
If you look at the test run below only after the first 2 commits, you can see that the test does not pass because there are still modules being used that don't work with ES5: https://github.com/webpack/webpack-dev-server/pull/2658/files/d01ac6855c0bc20f23054c9ca8832eea2539588d
Breaking Changes
None, client API is the same
Additional Info