feat: use compiler.platform to determine the target#5647
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5647 +/- ##
==========================================
+ Coverage 83.52% 83.54% +0.01%
==========================================
Files 11 11
Lines 1936 1920 -16
Branches 716 706 -10
==========================================
- Hits 1617 1604 -13
+ Misses 286 283 -3
Partials 33 33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lib/Server.js
Outdated
|
|
||
| return webTargets.includes(/** @type {string} */ (compiler.options.target)); | ||
| return ( | ||
| compiler.platform?.web || compiler.options.externalsPresets?.web || false |
There was a problem hiding this comment.
I think we can up minimum webpack version and don't use compiler.options.externalsPresets?.web all all, also we always have compiler.platform
There was a problem hiding this comment.
That sounds good to me. We’ve already bumped the minimum version to webpack 5.101, I just wasn’t sure if you also wanted to remove the externalPresets part.
There was a problem hiding this comment.
Yeah, we don't need externalPresets anymore here, only platform, it was literally design for such cases
lib/Server.js
Outdated
| } | ||
|
|
||
| return webTargets.includes(/** @type {string} */ (compiler.options.target)); | ||
| return compiler.platform?.web || false; |
There was a problem hiding this comment.
Let's remove ? here, you got it because webpack compiler can be Compiler or MultiCompiler, so you need
if (Array.isArray(compiler)) {
return compiler.some((r) => compiler.platform.web);
}
return compiler.platform.web;
There was a problem hiding this comment.
In fact, the "?" is not necessary. And when this function is called, it will be a compiler, not a multi-compiler (
webpack-dev-server/lib/Server.js
Line 605 in a4012b6
webpack-dev-server/lib/Server.js
Line 1698 in a4012b6
| } | ||
|
|
||
| return webTargets.includes(/** @type {string} */ (compiler.options.target)); | ||
| return compiler.platform.web || false; |
There was a problem hiding this comment.
Oh, forgot, we need to improve support of universal target too, i.e. target: "node" | "web", i..e when web === null && node === null, but in our client code we should not run code when it is node environment and run when it is web , but let's do it in other PR
|
Feel free to merge |
2 participants
Summary
The use of
compiler.platformis available starting from webpack 5.92.0 (webpack/webpack@e11fb12).The logic that added WebSocket communication when a
conditionNamesentry includedbrowserhas been removed. However, I’m not sure whether we also want to remove the behavior related toexternalsPresets. Do we want to remove that as well?If you want to know where it determines whether the web is available for a specific target, take a look here (https://github.com/webpack/webpack/blob/c835794b7fcd9f1e2f6750ae94c95aadf23a63dd/lib/config/target.js#L101)
What kind of change does this PR introduce?
refactor, and feat
Did you add tests for your changes?
Does this PR introduce a breaking change?
If relevant, what needs to be documented once your changes are merged or what have you already documented?