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

WIP: Event based refactor #17

Closed
wants to merge 33 commits into from
Closed

WIP: Event based refactor #17

wants to merge 33 commits into from

Conversation

adam-h
Copy link
Collaborator

@adam-h adam-h commented Oct 30, 2017

Continuation of #11

This is a WIP of an event based interface to Firehose, replacing the current callback interface completely.

@nguyenj What do you think about this?

TODO:

  • Multiplexed transports refactored
  • Add multiplex option to Connection (rather than having a MultiplexedConnection
  • Add a whole slew of tests (only part of WebSocketTransport is done so far)
  • Define what exactly each event means
    • Needs more work around 'Closed', 'Failed', 'Disconnected' and ways to distinguish errors which are fatal, which will try reconnecting and which were expected (we asked it to close nicely)

nguyenj and others added 30 commits August 24, 2017 16:57
This reverts commit 61ce1e5.

Without JSON.stringify, the variables being replaced ends up as a number
type or a variable rather than a string type value.
Alos cleanup some of the decaffeinate stuff
WebSocketTransport is upgraded and working with a basic manual test, needs automated tests

LongPollTransport is mostly upgraded and needs testing
@nguyenj
Copy link
Contributor

nguyenj commented Oct 30, 2017

@adam-h looking goooood.

Lets you use chrome://inspect with node8
@steel
Copy link
Member

steel commented Nov 7, 2017

This is a good start. I'd advocate to include these in this refactor.
#16
#15

@adam-h
Copy link
Collaborator Author

adam-h commented Nov 8, 2017

@steel Definately #15 , I'm not sure about #16 - started a chat there

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