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 use with browserify/webpack by adding back browser shim. #661

Closed
wants to merge 1 commit into from

Conversation

STRML
Copy link
Contributor

@STRML STRML commented Jan 9, 2016

The shim now errors when you try to use it. Its sole purpose is to prevent
browserify/webpack from trying to bundle the WebsocketServer, which will
obviously fail when trying to bundle 'fs', 'tls', et al.

This fixes some of the problems affecting users in socketio/engine.io-client#450, and fixes compatibility with browserify/webpack without having to explicitly ignore or shim 'ws'.

The shim now errors when you try to use it. Its sole purpose is to prevent
browserify/webpack from trying to bundle the WebsocketServer, which will
obviously fail when trying to bundle 'fs', 'tls', et al.
@3rd-Eden 3rd-Eden closed this Jan 9, 2016
@3rd-Eden
Copy link
Member

3rd-Eden commented Jan 9, 2016

Sorry, not going to bundle any sort of browser code here. Build systems should learn how to ignore optional dependencies instead of forcing packages to become compatible the build systems.

@STRML
Copy link
Contributor Author

STRML commented Jan 9, 2016

Why make every browserify/webpack user ignore 'ws' in order to use any
module that requires it? This simple file does not imply support for
browsers, but simply fixes bundling since bundlers cannot know not to
ignore 'ws' without specific instructions, which is why the browser field
exists in package.json.

This is not about optional dependencies, this is about breaking bundlers
because they will try to require all of this module's dependencies, such as
'fs' and 'tls'.
On Jan 9, 2016 12:51 PM, "Arnout Kazemier" notifications@github.com wrote:

Sorry, not going to bundle any sort of browser code here. Build systems
should learn how to ignore optional dependencies instead of forcing
packages to become compatible with them.


Reply to this email directly or view it on GitHub
#661 (comment).

@STRML
Copy link
Contributor Author

STRML commented Jan 9, 2016

This is putting consumers of this module in a bad situation (such as engine-io.client) as they need to upgrade to 1.0.1 asap due to the security bug, but now are forced to make a breaking change not only because of the optionalDependencies, but because the removal of browser.js broke bundlers for no good reason.

Expecting every consumer of this module to blacklist it in their bundlers is unnecessary when it can be easily fixed with this PR. This PR does not imply support, it throws an error if the module is used. It simply fixes bundling.

@rstacruz
Copy link

👍 on this as well. If it won't be supported, at least make it break. It took me a day to figure out why my project was suddenly having bufferutil and utf-8-validate errors: it turns out ws was a dependency 4 levels deep.

@dcousens
Copy link
Contributor

@3rd-Eden could you clarify if this module works at all with browserify now? Or should I just avoid using it?

@STRML
Copy link
Contributor Author

STRML commented Jan 29, 2016

I too am disappointed with how this has been closed with little/no discussion outside of "we don't want to support it". Why don't you want to support it, what cost is there to supporting it, and what's wrong with creating a tiny shim that basically says "don't use this", but doesn't break browserify/webpack builds like master does currently?

@andyinabox
Copy link

+1 yeah to me this seems to be a case of philosophy getting in the way of common usage. i really don't understand the decision to make this module a huge pain to use in the browser via browserify/webpack.

@bkonkle
Copy link

bkonkle commented Feb 28, 2017

+1 - actively removing conveniences in order to push an agenda hurts developers

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.

None yet

6 participants