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

Use built version os sockjs-client #1148

Merged
merged 1 commit into from Oct 16, 2017
Merged

Conversation

filipesilva
Copy link
Contributor

@filipesilva filipesilva commented Oct 16, 2017

What kind of change does this PR introduce?

Bugfix

Did you add or update the examples/?
Yes.

Summary

webpack-dev-server bundles sockjs-client from source. sockjs-client sources assume a node environment however, and will fail to run if global is not available.

By bundling sockjs-client from source, the node environment requirement from sockjs-client will also be required of the app being served and thus it is not possible to have node.global = false in the user app being bundled.

This fix uses the built sockjs-client instead of the sources.

Does this PR introduce a breaking change?

No.

Other information
Fix #1147

@codecov
Copy link

codecov bot commented Oct 16, 2017

Codecov Report

Merging #1148 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1148   +/-   ##
======================================
  Coverage    76.8%   76.8%           
======================================
  Files           5       5           
  Lines         470     470           
  Branches      151     151           
======================================
  Hits          361     361           
  Misses        109     109

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 32412bb...fe5e48c. Read the comment docs.

@@ -0,0 +1,9 @@
# CLI - node false
Copy link
Contributor

Choose a reason for hiding this comment

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

these are more for "how to" for users, but there's no harm in adding this. the tests that matter are in /test. no worries if you can't get to adding one there, we can do it after merging.

@filipesilva
Copy link
Contributor Author

@shellscape I removed the example, I don't think it's a real user example.

Regarding the real tests in /test, to test node: false doesn't throw a runtime error, a e2e test would be needed. It looks like there is no test where a browser is actually used. Instead I added node: false to the simple config to ensure that stuff in general still works with it, and that it's not needed for normal webpack-dev-server functioning. WDYT?

@shellscape
Copy link
Contributor

Yeah good point; we would actually need a browser for this. I'd leave the example in there as a means to test that change at the moment, even though it's not a traditional example. We can look into headless browser testing for the client in the future.

`webpack-dev-server` bundles `sockjs-client` from source. `sockjs-client` sources assume a node environment however, and will fail to run if `global` is not available.

By bundling `sockjs-client` from source, the node environment requirement from `sockjs-client` will also be required of the app being served and thus it is not possible to have `node.global = false` in the user app being bundled.

This fix uses the built `sockjs-client` instead of the sources.

Fix webpack#1147
@filipesilva
Copy link
Contributor Author

Done, re-added it.

@shellscape shellscape merged commit 06df2f4 into webpack:master Oct 16, 2017
@shellscape
Copy link
Contributor

Thanks much :)

@filipesilva filipesilva deleted the patch-1 branch October 16, 2017 18:51
@filipesilva
Copy link
Contributor Author

Thank you for the quick review!

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.

Runtime error when node.global is set to false.
2 participants