pladaria / reconnecting-websocket

Reconnecting WebSocket. For Web, React Native, cli (Node.js)
MIT License
1.22k stars 197 forks source link

Set no delay for the first connection #94

Closed quentinus95 closed 5 years ago

coveralls commented 5 years ago

Coverage Status

Coverage remained the same at 94.324% when pulling ad9d0330685087276b24094a9f9f5fef94f2792e on quentinus95:patch-1 into c64d32fbeaf70199e7b82817476f4d9f3e35ef39 on pladaria:master.

quentinus95 commented 5 years ago

Do you see a way to cover it?

I tried:

const wss = new WebSocketServer({port: PORT});
const ws = new ReconnectingWebSocket(URL);

t.is(ws._getNextDelay(), 0);

ws.close();
wss.close();
phil-flyclops commented 5 years ago

It would be a little flaky but you could do something like take a time reading before creating the websocket, create the websocket with a high minReconnectionDelay, and then do an open listener that asserts that lower than a reasonably short time has passed. Not really sure if there is a cleaner way.

Also, this behavior is blocking me from being able to use this library, so let me know if I can do anything to help ferry this through

curran commented 5 years ago

@quentinus95 I haven't had a chance to dig into the internals here and see how it could be covered by tests, but you must have verified the behavior manually? Maybe try to think of a way to re-create your manual verification within the tests. Perhaps using setTimeout to check the actual timing.

@phil-flyclops Maybe try adding a test case for it?

Haven't heard yet from @pladaria , his approval would seal the deal.

natew commented 5 years ago

This is a pretty big fix, would really appreciate seeing it get merged.

AlanVerne commented 5 years ago

It's still not merged?

quentinus95 commented 5 years ago

Approved but not merged....

trycoon commented 5 years ago

Please merge. :-)

curran commented 5 years ago

Looks like @pladaria is no longer maintaining this lib. Any active forks?

quentinus95 commented 5 years ago

I can fork it and deploy it to npm with the fix if you want me to.

arnarthor commented 5 years ago

@quentinus95 Publishing this as a fork to npm would be great, or even just commit the compiled files to your fork so people could install it from your github until this is merged.

dawnmist commented 5 years ago

For situations like this, the patch-package module is fantastic. It lets you put a temporary patch on an npm module as part of the install process for that module (so it is reapplied each time you install/delete/update any modules). It means that you don't need someone to publish a fork if you're able to make the change to the compiled module code yourself. Since this is a pretty simple change, it's relatively easy to apply it to each of the dist folder files.

dgreensp commented 5 years ago

I hope this can be merged soon :(

bj00rn commented 5 years ago

@pladaria thanks for a great lib! are you still maintaining?

can we expect a merge+release anytime soon?

heavyk commented 5 years ago

if you're going to put out a new version, can you include this fix as well?

pladaria commented 5 years ago

Thanks!