- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.5k
          Use built version os sockjs-client
          #1148
        
          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
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     109Continue 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-serverbundlessockjs-clientfrom source.sockjs-clientsources assume a node environment however, and will fail to run ifglobalis not available.By bundling
sockjs-clientfrom source, the node environment requirement fromsockjs-clientwill also be required of the app being served and thus it is not possible to havenode.global = falsein the user app being bundled.This fix uses the built
sockjs-clientinstead of the sources.Does this PR introduce a breaking change?
No.
Other information
Fix #1147