Skip to content
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

Merged
merged 2 commits into from
Apr 17, 2019

Conversation

mlrawlings
Copy link
Contributor

@mlrawlings mlrawlings commented Apr 9, 2019

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

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" or target:"webworker").

With target:"node", the following error is thrown:

urlParts.port = self.location.port;
                ^
ReferenceError: self is not defined

From

urlParts.port = self.location.port;

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 (when options.hot or options.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

@jsf-clabot
Copy link

jsf-clabot commented Apr 9, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Apr 9, 2019

Codecov Report

Merging #1775 into master will increase coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
lib/utils/addEntries.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58d1682...ca0334d. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a 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 👍

@alexander-akait
Copy link
Member

/cc @hiroppy

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting note

@mlrawlings
Copy link
Contributor Author

mlrawlings commented Apr 9, 2019

@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): inlineClient and inlineHot. These options control whether, when inline is true, which configurations get the client runtime and hot runtimes inlined.

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.

@alexander-akait
Copy link
Member

Looks good, but inlineClient and inclideHot is very misleading, maybe injectClient and injectHot?

@mlrawlings
Copy link
Contributor Author

Updated. I like inject better as well.

@alexander-akait
Copy link
Member

/cc @hiroppy

Copy link
Member

@hiroppy hiroppy left a 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!

@hiroppy
Copy link
Member

hiroppy commented Apr 16, 2019

@mlrawlings Could you rebase?

@hiroppy
Copy link
Member

hiroppy commented Apr 16, 2019

BTW, I'll add an example for this after this pr is merged.

@alexander-akait alexander-akait merged commit cf4d0d0 into webpack:master Apr 17, 2019
@alexander-akait
Copy link
Member

Thanks great job!

@EECOLOR
Copy link

EECOLOR commented Apr 29, 2019

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:

 ReferenceError: self is not defined

@EECOLOR
Copy link

EECOLOR commented May 8, 2019

Does it make sense to still have the HotModuleReplacementPlugin injected for non web targets?

@FallingSnow
Copy link

I'm running into this issue while using web audio workers at

self.addEventListener("beforeunload", () => {
status.isUnloading = true;
});

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 webworker but the error persists.

@snitin315
Copy link
Member

snitin315 commented Sep 1, 2023

@FallingSnow Can you please create an issue with proper steps to reproduce?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants