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

Two small modifications (MessageEvent object + retry field handling) #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xfra35
Copy link

@xfra35 xfra35 commented Jul 6, 2012

Hi, I've tested EventSource.js in IE7/8/9 and it works great. Thanks for your work !

I slightly modified the code as follows :

  • addition of the "type" property to the MessageEvent object
  • handling of "retry" field name

(tested in IE7/8/9)

@remy
Copy link
Owner

remy commented Jul 6, 2012

The event.type looks good, but the interval isn't quite right. Note that the way this eventsource implementation works is that when the data comes in, the connection is dropped, so it has to automatically reestablish the connection. This should be pretty much right away.

Also the interval value shouldn't be available in the eventsource object as it's internal to our polyfill and shouldn't be exposed to the developer.

My gut feeling is the interval retry should be used here https://github.com/xfra35/polyfills/blob/18c85ae81ca905c2fa8255d9f4fff94b31a791d2/EventSource.js#L96 - but definitely needs testing with a server that maybe gets shutdown to test the retry backoff.

@xfra35
Copy link
Author

xfra35 commented Jul 6, 2012

"Also the interval value shouldn't be available in the eventsource object as it's internal to our polyfill and shouldn't be exposed to the developer." >> you're totally right. I didn't think about that aspect.

I just moved it back as originally (interval variable) and updated the "retry" line accordingly. I made a test with a server shutdown. It kept on trying every 2s (my retry value) and retrieved the connection correctly when the server went back up.

Concerning the default polling interval, don't you think 500ms is too short ? I've read somewhere that the usual default polling value is 3000ms although I can't find that value in the w3c specs.

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.

2 participants