-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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(addEntries): only add client entry to web targets #1775
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1775 +/- ##
=========================================
+ Coverage 87.58% 87.78% +0.2%
=========================================
Files 9 9
Lines 596 606 +10
Branches 179 184 +5
=========================================
+ Hits 522 532 +10
Misses 62 62
Partials 12 12
Continue to review full report at Codecov.
|
7e3cff7
to
b03d176
Compare
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.
Thanks for PR, looks very good 👍
/cc @hiroppy |
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.
Interesting note
@evilebottnawi I see your concern and I also realized the target can also be a function which complicates things. I've just pushed a commit that adds two options (plus tests): new DevServer(compiler, {
/* default: web targets only */
inlineClient: (config) => (
config.target === 'web' ||
config.target === 'webworker' ||
config.target == null
)
}); new DevServer(compiler, {
/* add to all compilers */
inlineClient: true
}); new DevServer(compiler, {
/* add to no compilers */
inlineClient: false
}); This would allow users to choose to inline based on any config value. The name option in particular could be useful to target specific compilations. new DevServer(compiler, {
/* add to all compilers (as a function) */
inlineClient: (config) => config.name !== 'Server'
}); Most users shouldn't ever have to set these options, but they're there if someone needs them. For the vast majority of users, webpack-dev-server should continue to work out-of-the-box. |
Looks good, but |
Updated. I like |
/cc @hiroppy |
b9fde21
to
6bb5d5b
Compare
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 confirmed, LGTM. Thank you!
@mlrawlings Could you rebase? |
BTW, I'll add an example for this after this pr is merged. |
939a1d7
to
bf5adc2
Compare
bf5adc2
to
858b2eb
Compare
858b2eb
to
ca0334d
Compare
Thanks great job! |
Awesome, it took a few hours to see this was the problem, but I was very happy to see it has already been solved and merged. Any indication on when this will be released? Thanks! For future reference, the error I received:
|
Does it make sense to still have the |
I'm running into this issue while using web audio workers at webpack-dev-server/client-src/index.js Lines 149 to 151 in 540c438
ReferenceError: self is not defined I have tried the following in my webpack config but the issue remains. output: {
globalObject: "globalThis"
} I also tried setting the target to |
@FallingSnow Can you please create an issue with proper steps to reproduce? |
For Bugs and Features; did you add new tests?
Yes
Motivation / Use-Case
When using an universal/isomorphic multicompiler setup and using the default setting of
inline:true
, the client runtime gets injected into the entries for each compilation. However, as far as I can tell, the client runtime is only designed to work in the browser (target:"web"
ortarget:"webworker"
).With
target:"node"
, the following error is thrown:From
webpack-dev-server/client-src/default/index.js
Line 46 in 99e8db0
Changes
This PR updates
addEntries
to check the compilation target for each compiler. Since the client runtime is only designed for web targets, it is only prepended to those compilations. The HMR runtimes are still prepended for all compile targets (whenoptions.hot
oroptions.hotOnly
are set), because as far as I can tell, these should work regardless of the compilation target.Breaking Changes
None, if my assumptions are correct.
Additional Info
Related: #1256 #1338 #1441