fix: Align `TimerManager` sequential ids and function error handling with web standard #51500

kitten · 2025-05-21T14:33:21Z

Summary:

Calls to create timers should return sequential ids (integers greater than zero in the spec's words). This regressed in the TimerManager implementation, which instead starts at zero inclusively.

This has two side-effects for code assuming a spec-compliant implementation of setTimeout and setInterval:

Calls to clearTimeout(0) or clearInterval(0) will potentially cancel scheduled timers, although it's supposed to be a noop
Predicates like if (timeoutId) will fail since they assume non-negative ids

The change in this PR is to align with WHATWG HTML 8.6.2 (Timers): https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timers

otherwise, let id be an implementation-defined integer that is greater than zero and does not already exist in global's map of setTimeout and setInterval IDs.

Specifically,

we should return 0 to indicate that no timer was scheduled
we should start generating timer IDs at 1 instead of 0

This was previously raised in review comments here: https://github.com/facebook/react-native/pull/45092/files#r1650790008

The spec-incompliant behaviour was raised in an issue here: apollographql/apollo-client#12632 (comment)

This PR does not,

add bounds checking on timerIndex_ and add a search of an available id that isn't in the unordered map
exclude 0 from being an accepted TimerHandle in TimerManager::createTimer or TimerManager::deleteTimer since the above bounds checking hasn't been added either

Changelog:

[GENERAL] [FIXED] - Align timer IDs and timer function argument error handling with web standards.

Test Plan:

Run setTimeout / setInterval; before applied changes the timeout for the first timer will be 0
Run setTimeout(null); before applied changes the timer ID will be non-zero
Run setInterval(null); before applied changes an error will be thrown rather than 0 being returned

As per WHATWG HTML 8.6.1 (Timers) ids must be greater than zero, i.e. start at 1

facebook-github-bot · 2025-05-21T15:04:57Z

@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cipolleschi

Thanks for the fix! :D

facebook-github-bot · 2025-05-22T11:47:02Z

@rshest merged this pull request in 480a464.

react-native-bot · 2025-05-22T11:47:04Z

This pull request was successfully merged by @kitten in 480a464

^{When will my fix make it into a release? | How to file a pick request?}

…with web standard (#51500) Summary: Calls to create timers should return sequential ids (integers greater than zero in the spec's words). This regressed in the `TimerManager` implementation, which instead starts at zero inclusively. This has two side-effects for code assuming a spec-compliant implementation of `setTimeout` and `setInterval`: - Calls to `clearTimeout(0)` or `clearInterval(0)` will potentially cancel scheduled timers, although it's supposed to be a noop - Predicates like `if (timeoutId)` will fail since they assume non-negative ids The change in this PR is to align with WHATWG HTML 8.6.2 (Timers): https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timers > otherwise, let id be an [implementation-defined](https://infra.spec.whatwg.org/#implementation-defined) integer that is **greater than zero** and does not already [exist](https://infra.spec.whatwg.org/#map-exists) in global's [map of setTimeout and setInterval IDs](https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#map-of-settimeout-and-setinterval-ids). Specifically, - we should return `0` to indicate that no timer was scheduled - we should start generating timer IDs at `1` instead of `0` This was previously raised in review comments here: https://github.com/facebook/react-native/pull/45092/files#r1650790008 The spec-incompliant behaviour was raised in an issue here: apollographql/apollo-client#12632 (comment) This PR does not, - add bounds checking on `timerIndex_` and add a search of an available id that isn't in the unordered map - exclude `0` from being an accepted `TimerHandle` in `TimerManager::createTimer` or `TimerManager::deleteTimer` since the above bounds checking hasn't been added either ## Changelog: [GENERAL] [FIXED] - Align timer IDs and timer function argument error handling with web standards. Pull Request resolved: #51500 Test Plan: - Run `setTimeout` / `setInterval`; before applied changes the timeout for the first timer will be `0` - Run `setTimeout(null)`; before applied changes the timer ID will be non-zero - Run `setInterval(null)`; before applied changes an error will be thrown rather than `0` being returned Reviewed By: cipolleschi Differential Revision: D75145909 Pulled By: rshest fbshipit-source-id: 6646439abd29cf3cfa9e5cf0a57448e3b7cd1b48

react-native-bot · 2025-05-23T09:53:30Z

This pull request was successfully merged by @kitten in 24131c6

^{When will my fix make it into a release? | How to file a pick request?}

…with web standard (#51500) Summary: Calls to create timers should return sequential ids (integers greater than zero in the spec's words). This regressed in the `TimerManager` implementation, which instead starts at zero inclusively. This has two side-effects for code assuming a spec-compliant implementation of `setTimeout` and `setInterval`: - Calls to `clearTimeout(0)` or `clearInterval(0)` will potentially cancel scheduled timers, although it's supposed to be a noop - Predicates like `if (timeoutId)` will fail since they assume non-negative ids The change in this PR is to align with WHATWG HTML 8.6.2 (Timers): https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timers > otherwise, let id be an [implementation-defined](https://infra.spec.whatwg.org/#implementation-defined) integer that is **greater than zero** and does not already [exist](https://infra.spec.whatwg.org/#map-exists) in global's [map of setTimeout and setInterval IDs](https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#map-of-settimeout-and-setinterval-ids). Specifically, - we should return `0` to indicate that no timer was scheduled - we should start generating timer IDs at `1` instead of `0` This was previously raised in review comments here: https://github.com/facebook/react-native/pull/45092/files#r1650790008 The spec-incompliant behaviour was raised in an issue here: apollographql/apollo-client#12632 (comment) This PR does not, - add bounds checking on `timerIndex_` and add a search of an available id that isn't in the unordered map - exclude `0` from being an accepted `TimerHandle` in `TimerManager::createTimer` or `TimerManager::deleteTimer` since the above bounds checking hasn't been added either ## Changelog: [GENERAL] [FIXED] - Align timer IDs and timer function argument error handling with web standards. Pull Request resolved: #51500 Test Plan: - Run `setTimeout` / `setInterval`; before applied changes the timeout for the first timer will be `0` - Run `setTimeout(null)`; before applied changes the timer ID will be non-zero - Run `setInterval(null)`; before applied changes an error will be thrown rather than `0` being returned Reviewed By: cipolleschi Differential Revision: D75145909 Pulled By: rshest fbshipit-source-id: 6646439abd29cf3cfa9e5cf0a57448e3b7cd1b48

react-native-bot · 2025-06-04T09:39:40Z

This pull request was successfully merged by @kitten in 942422c

^{When will my fix make it into a release? | How to file a pick request?}

…with web standard (#51500) Summary: Calls to create timers should return sequential ids (integers greater than zero in the spec's words). This regressed in the `TimerManager` implementation, which instead starts at zero inclusively. This has two side-effects for code assuming a spec-compliant implementation of `setTimeout` and `setInterval`: - Calls to `clearTimeout(0)` or `clearInterval(0)` will potentially cancel scheduled timers, although it's supposed to be a noop - Predicates like `if (timeoutId)` will fail since they assume non-negative ids The change in this PR is to align with WHATWG HTML 8.6.2 (Timers): https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timers > otherwise, let id be an [implementation-defined](https://infra.spec.whatwg.org/#implementation-defined) integer that is **greater than zero** and does not already [exist](https://infra.spec.whatwg.org/#map-exists) in global's [map of setTimeout and setInterval IDs](https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#map-of-settimeout-and-setinterval-ids). Specifically, - we should return `0` to indicate that no timer was scheduled - we should start generating timer IDs at `1` instead of `0` This was previously raised in review comments here: https://github.com/facebook/react-native/pull/45092/files#r1650790008 The spec-incompliant behaviour was raised in an issue here: apollographql/apollo-client#12632 (comment) This PR does not, - add bounds checking on `timerIndex_` and add a search of an available id that isn't in the unordered map - exclude `0` from being an accepted `TimerHandle` in `TimerManager::createTimer` or `TimerManager::deleteTimer` since the above bounds checking hasn't been added either ## Changelog: [GENERAL] [FIXED] - Align timer IDs and timer function argument error handling with web standards. Pull Request resolved: #51500 Test Plan: - Run `setTimeout` / `setInterval`; before applied changes the timeout for the first timer will be `0` - Run `setTimeout(null)`; before applied changes the timer ID will be non-zero - Run `setInterval(null)`; before applied changes an error will be thrown rather than `0` being returned Reviewed By: cipolleschi Differential Revision: D75145909 Pulled By: rshest fbshipit-source-id: 6646439abd29cf3cfa9e5cf0a57448e3b7cd1b48

react-native-bot · 2025-07-01T13:46:56Z

This pull request was successfully merged by @kitten in 26b70c2

^{When will my fix make it into a release? | How to file a pick request?}

…with web standard (#51500) Summary: Calls to create timers should return sequential ids (integers greater than zero in the spec's words). This regressed in the `TimerManager` implementation, which instead starts at zero inclusively. This has two side-effects for code assuming a spec-compliant implementation of `setTimeout` and `setInterval`: - Calls to `clearTimeout(0)` or `clearInterval(0)` will potentially cancel scheduled timers, although it's supposed to be a noop - Predicates like `if (timeoutId)` will fail since they assume non-negative ids The change in this PR is to align with WHATWG HTML 8.6.2 (Timers): https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timers > otherwise, let id be an [implementation-defined](https://infra.spec.whatwg.org/#implementation-defined) integer that is **greater than zero** and does not already [exist](https://infra.spec.whatwg.org/#map-exists) in global's [map of setTimeout and setInterval IDs](https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#map-of-settimeout-and-setinterval-ids). Specifically, - we should return `0` to indicate that no timer was scheduled - we should start generating timer IDs at `1` instead of `0` This was previously raised in review comments here: https://github.com/facebook/react-native/pull/45092/files#r1650790008 The spec-incompliant behaviour was raised in an issue here: apollographql/apollo-client#12632 (comment) This PR does not, - add bounds checking on `timerIndex_` and add a search of an available id that isn't in the unordered map - exclude `0` from being an accepted `TimerHandle` in `TimerManager::createTimer` or `TimerManager::deleteTimer` since the above bounds checking hasn't been added either ## Changelog: [GENERAL] [FIXED] - Align timer IDs and timer function argument error handling with web standards. Pull Request resolved: #51500 Test Plan: - Run `setTimeout` / `setInterval`; before applied changes the timeout for the first timer will be `0` - Run `setTimeout(null)`; before applied changes the timer ID will be non-zero - Run `setInterval(null)`; before applied changes an error will be thrown rather than `0` being returned Reviewed By: cipolleschi Differential Revision: D75145909 Pulled By: rshest fbshipit-source-id: 6646439abd29cf3cfa9e5cf0a57448e3b7cd1b48

react-native-bot · 2025-07-09T10:33:29Z

This pull request was successfully merged by @kitten in 157cf11

^{When will my fix make it into a release? | How to file a pick request?}

kitten added 3 commits May 21, 2025 15:13

Update TimerManager timerIndex_ to start at 1 instead of 0

deab62e

As per WHATWG HTML 8.6.1 (Timers) ids must be greater than zero, i.e. start at 1

Return 0 instead of incremented timer id

a3d0ccb

Remove invalid thrown error in setInterval and instead return 0

935a163

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Expo Partner: Expo Partner labels May 21, 2025

kitten changed the title ~~fix: Align TimerManager sequential ids and function error handling on with web standard~~ fix: Align TimerManager sequential ids and function error handling with web standard May 21, 2025

facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label May 21, 2025

test: Expect 0 as return value for invalid setInterval handler argument

05d6d7b

phryneas mentioned this pull request May 21, 2025

useFragment debounce logic clears timer ID 0 and unintentionally cancels unrelated timers apollographql/apollo-client#12632

Closed

kitten added 2 commits May 21, 2025 15:49

test: Expect 0 as return value for invalid setTimeout handler argument

54098aa

test: Timer ID should start at 1 not 0

800885d

cipolleschi approved these changes May 21, 2025

View reviewed changes

facebook-github-bot closed this in 480a464 May 22, 2025

facebook-github-bot added the Merged This PR has been merged. label May 22, 2025

cipolleschi mentioned this pull request Jun 4, 2025

[Local] Fix CI for 0.79-stable #51804

Merged

cipolleschi mentioned this pull request Jun 16, 2025

[RN][Codegen] Backports fix: exclusion of selectively disabled libraries from codegen generation #52040

Merged

cipolleschi mentioned this pull request Oct 20, 2025

[RN]Downgrade CMake to fix CI #54210

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Align `TimerManager` sequential ids and function error handling with web standard #51500

fix: Align `TimerManager` sequential ids and function error handling with web standard #51500

Uh oh!

kitten commented May 21, 2025 •

edited

Loading

Uh oh!

facebook-github-bot commented May 21, 2025

Uh oh!

cipolleschi left a comment

Uh oh!

facebook-github-bot commented May 22, 2025

Uh oh!

react-native-bot commented May 22, 2025

Uh oh!

react-native-bot commented May 23, 2025

Uh oh!

react-native-bot commented Jun 4, 2025

Uh oh!

react-native-bot commented Jul 1, 2025

Uh oh!

react-native-bot commented Jul 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix: Align TimerManager sequential ids and function error handling with web standard #51500

fix: Align TimerManager sequential ids and function error handling with web standard #51500

Uh oh!

Conversation

kitten commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

Changelog:

Test Plan:

Uh oh!

facebook-github-bot commented May 21, 2025

Uh oh!

cipolleschi left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented May 22, 2025

Uh oh!

react-native-bot commented May 22, 2025

Uh oh!

react-native-bot commented May 23, 2025

Uh oh!

react-native-bot commented Jun 4, 2025

Uh oh!

react-native-bot commented Jul 1, 2025

Uh oh!

react-native-bot commented Jul 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix: Align `TimerManager` sequential ids and function error handling with web standard #51500

fix: Align `TimerManager` sequential ids and function error handling with web standard #51500

kitten commented May 21, 2025 •

edited

Loading