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

Allow client to always use native setTimeout function. #1479

Closed
wants to merge 7 commits into from

Conversation

vartan
Copy link
Contributor

@vartan vartan commented Jul 1, 2021

This allows socket.io client to be used in testing infrastructure (such as Karma) where the clock may be mocked.

Note: the socket.io.js file is the generated output of make socket.io.js, and should not be manually modified.

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behaviour

If the test infrastructure socket attempts to reconnect while a mock clock is installed, it will never reconnect. This happens when Karma is configured to use socket.io.

New behaviour

If the socket attempts to reconnect while the mock clock is installed and useNativeSetTimeout is set, then it will not use the mocked clock and will reconnect normally

Other information (e.g. related issues)

…cket.io client to be used in testing infrastructure (such as Karma) where the clock may be mocked.
@darrachequesne
Copy link
Member

@vartan thanks for the pull request! There are several usages of setTimeout in engine.io-client, do you think we should add the option there too?

@vartan
Copy link
Contributor Author

vartan commented Jul 2, 2021

Yeah, that seems like the right thing to do, I didn't realize that was split out of socket.io-client. Is there somewhere central you recommend I implement this? I'll take a look tomorrow.

@vartan
Copy link
Contributor Author

vartan commented Jul 8, 2021

I sent a PR to engine.io-client, I'll refactor this one once that is submitted. thanks again.

darrachequesne pushed a commit to socketio/engine.io-client that referenced this pull request Jul 30, 2021
This allows to control the behavior of mocked timers (@sinonjs/fake-timers),
depending on the value of the "useNativeTimers" option:

- true: use native setTimeout function
- false (default): use classic timers, that may be mocked

The "installTimerFunctions" method will also be used in the
`socket.io-client` package:

```
import { installTimerFunctions } from "engine.io-client/lib/util";
```

Note: we could also have put the method in its own library, but that
sounded a bit overkill

Related: socketio/socket.io-client#1479
darrachequesne pushed a commit to socketio/engine.io-client that referenced this pull request Jul 30, 2021
This allows to control the behavior of mocked timers (@sinonjs/fake-timers),
depending on the value of the "useNativeTimers" option:

- true: use native setTimeout function
- false (default): use classic timers, that may be mocked

The "installTimerFunctions" method will also be used in the
`socket.io-client` package:

```
import { installTimerFunctions } from "engine.io-client/lib/util";
```

Note: we could also have put the method in its own library, but that
sounded a bit overkill

Related: socketio/socket.io-client#1479
@vartan
Copy link
Contributor Author

vartan commented Aug 3, 2021

Hi Damien, I've updated this PR to use the changes in socketio/engine.io-client#672

@vartan
Copy link
Contributor Author

vartan commented Aug 3, 2021

I wonder if we should also bump up the version of engine.io-client to ensure the change is in

@vartan
Copy link
Contributor Author

vartan commented Aug 17, 2021

friendly ping

darrachequesne pushed a commit that referenced this pull request Aug 29, 2021
This allows to control the behavior of mocked timers (@sinonjs/fake-timers),
depending on the value of the "useNativeTimers" option:

- true: use native setTimeout function
- false (default): use classic timers, that may be mocked

Related: socketio/engine.io-client@5d1d5be
@darrachequesne
Copy link
Member

Merged as 4e1b656. I'll keep you updated once it is released.

@darrachequesne darrachequesne added this to the 4.2.0 milestone Aug 30, 2021
@darrachequesne
Copy link
Member

This was included in latest release: https://github.com/socketio/socket.io-client/releases/tag/4.2.0

Again, thanks for your work on this.

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