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

handle the case where self._interval is undefined #482

Closed
wants to merge 1 commit into from

Conversation

lacker
Copy link

@lacker lacker commented Jun 20, 2019

In some situations, Node returns undefined for setInterval. This just adds a check to make sure we don't access properties on undefined

@lacker
Copy link
Author

lacker commented Jun 20, 2019

This PR fixes the problem that I reported in #481

@t-mullen
Copy link
Collaborator

I'm not against merging this if JSDom isn't being fixed. Seems like a potentially breaking change on their side.

@lacker
Copy link
Author

lacker commented Jul 19, 2019

Yeah, I don't know how long it will take to change JSDom - this is probably a low priority fix on their end. And it seems pretty harmless to fix it here. So IMO it would be clean just to merge this fix.

@nazar-pc
Copy link
Collaborator

I'd actually be against merging hacks like this, especially since it is possible to hack around it without changing simple-peer.

@lacker
Copy link
Author

lacker commented Jul 19, 2019

For what it's worth, it's not that JSDom itself is so popular that simple-peer should support it per se, it's that when I was using simple-peer in conjunction with Jest unit testing, the tests were crashing frequently outside of my code, and I tracked it down to this problem. So fixing this would let simple-peer work together with Jest unit tests. That seems popular enough that it is worth compatibility IMO.

@t-mullen
Copy link
Collaborator

Monkey patching window.setInterval outside of simple-peer is the solution until JSDom fixes this. You should do this anyways since this behaviour may break other libraries.

I know it seems easier to include this hack in simple-peer, but realize that every single JS library will need to do this as well.

jsdom/jsdom#2617

From spec:

The setInterval() method must return the value returned by the timer initialization steps ...a user-agent-defined integer that is greater than zero that will identify the timeout to be set by this call in the list of active timers.

@t-mullen t-mullen closed this Dec 13, 2019
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.

None yet

3 participants