-
-
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
Use built version os sockjs-client
#1148
Conversation
Codecov Report
@@ 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.
|
@@ -0,0 +1,9 @@ | |||
# CLI - node false |
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.
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.
@shellscape I removed the example, I don't think it's a real user example. Regarding the real tests in |
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 #1147
Done, re-added it. |
Thanks much :) |
Thank you for the quick review! |
What kind of change does this PR introduce?
Bugfix
Did you add or update the
examples/
?Yes.
Summary
webpack-dev-server
bundlessockjs-client
from source.sockjs-client
sources assume a node environment however, and will fail to run ifglobal
is not available.By bundling
sockjs-client
from source, the node environment requirement fromsockjs-client
will also be required of the app being served and thus it is not possible to havenode.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