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

Checking socket functionality - compatibility with webpack/browserify #44

Merged
merged 1 commit into from
Feb 11, 2016

Conversation

anacronw
Copy link

The code was erroring out for me while on node v4.0 I noticed that at the time of development, it may have attempted to support some now-deprecated functionality

Actually I misunderstood the problem - its certainly not deprecated of course :)

The issue was that I was using this library with webpack which doesn't seem to emulate node's net module correctly when consumed through commonjs

This PR simply checks for the existence of those methods before calling them

@anacronw anacronw closed this Dec 17, 2015
@anacronw anacronw reopened this Dec 17, 2015
@aslakhellesoy
Copy link
Contributor

Are you trying to run this library in a browser?

@anacronw
Copy link
Author

Yes exactly, for the reason being that it is packaged through commonjs and it seems to work fine on IE. Of course, a lot of the browsers have innate support of EventSource :)

I also intend to run it with node using as well of course

@aslakhellesoy
Copy link
Contributor

Why? Your browser has a built-in EventSource class. I recommend using a polyfill specifically intended for old browsers instead.

https://github.com/Yaffle/EventSource/

@anacronw
Copy link
Author

I understand, but it turns out your package is elegantly the one I wish to use. I'm writing a node + browser abstraction to a proprietary API. Its written in CommonJS, but is able to be browserified/webpacked for the browser.

I've actually tried the top most popular implementations of EventSource including this library
event-source-polyfill
eventsource-polyfill

The first (the one you recommended) isn't packaged for commonjs, so its less than ideal in my case. The second is just buggy and does not properly long poll from what I can tell.

With this merged in, your library surprisingly actually supports both browser and node use case. Of course, while it doesn't check for the existing presence of EventSource in the cases that it doesn't need to be polyfilled - I can do that on my end.

Thanks!

@anacronw anacronw changed the title Checking socket functionality - forward compatibility with node v4.0 Checking socket functionality - compatibility with webpack/browserify Dec 17, 2015
@piranna
Copy link
Member

piranna commented Jan 4, 2016

it doesn't check for the existing presence of EventSource in the cases that it doesn't need to be polyfilled

It would be cool if the library would be able to check for its existence and use browser native ones when available :-)

@aslakhellesoy aslakhellesoy merged commit 64975ff into EventSource:master Feb 11, 2016
@anacronw
Copy link
Author

thanks!

@aslakhellesoy
Copy link
Contributor

My pleasure. It's in 0.2.0 released earlier today

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.

3 participants