feat: support universal platform as a web target#5690
Conversation
|
| Name | Type |
|---|---|
| webpack-dev-server | Minor |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
There was a problem hiding this comment.
I'm not sure a unit test is worth it, but I'll add one. If you'd rather remove it, that's fine with me. Or if you'd prefer an E2E test, just let me know.
There was a problem hiding this comment.
Let's add one e2e test for this and wen can merge
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5690 +/- ##
==========================================
+ Coverage 88.93% 88.95% +0.01%
==========================================
Files 13 13
Lines 5922 5964 +42
==========================================
+ Hits 5267 5305 +38
- Misses 655 659 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
We have an issue even with universal, I'm sending a patch to the core because it's still trying to use require in the browser for HMR. |
|
Lol, it works locally |
… with newer webpack
bjohansebas
force-pushed
the
universal-target
branch
from
0d45636 to
0c9337b
Compare
…th older webpack versions
|
Lesson learned: always rebase |
| // For universal targets `webpack/hot/emitter` uses Node's `events`, | ||
| // which breaks in the browser. Swap it for webpack's `EventTarget` | ||
| // emitter when available. | ||
| if ( | ||
| compiler.options.output.module && | ||
| compiler.platform.web === null && | ||
| compiler.platform.node === null | ||
| ) { | ||
| let emitter; | ||
|
|
||
| try { | ||
| emitter = cjsRequire.resolve("webpack/hot/emitter-event-target.js"); | ||
| } catch { | ||
| // older webpack versions do not ship the `EventTarget` emitter | ||
| } | ||
|
|
||
| if (emitter) { | ||
| new webpack.NormalModuleReplacementPlugin( | ||
| /emitter(\.js)?$/, | ||
| (result) => { | ||
| if ( | ||
| /webpack[/\\]hot|webpack-dev-server[/\\]client/.test( | ||
| result.context, | ||
| ) | ||
| ) { | ||
| result.request = emitter; | ||
| } | ||
| }, | ||
| ).apply(compiler); | ||
| } | ||
| } |
There was a problem hiding this comment.
The same thing needs to be done on the v5 branch, but I'd prefer that we ship v6 first. After that, I can backport the patch to v5 to avoid having to do another rebase on next.
There was a problem hiding this comment.
It'll probably be a bit more involved for the v5 branch, but that's something to figure out when I put together that patch.
2 participants
Summary
We already supported this on
main—the[node, web]combination—but it was removed when theisWebTargetmethod was simplified. So I'm adding it back now, along with support for theuniversalplatform. If we were to raise the peer dependency version, we could simplify the function even further, but I don't think it's worth doing that yet.What kind of change does this PR introduce?
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?
Use of AI