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

[Scheduler] Get current time from performance.now in non-DOM environments #19532

Merged
merged 3 commits into from Aug 5, 2020

Conversation

emilisb
Copy link
Contributor

@emilisb emilisb commented Aug 5, 2020

Summary

Scheduler makes an assumption that performance.now does not exist in a non-DOM environment. React Native is such environment, however, latest RN versions do have support for performance.now (facebook/react-native@232517a).

This PR changes getCurrentTime implementation to use performance.now if it is defined, and fallback to Date.now if the function is missing.

This change is required for React Profiler API to work properly in React Native. Without this change, the Profiler in RN uses Date which is not so precise.

Test Plan

  • Tests are passing
  • Code lints
  • I built scheduler and tested the change in a React Native project. Profiler API now takes time from performance.now and the precision is greatly improved.

Screenshot 2020-08-05 at 14 42 40

@emilisb emilisb changed the title Get current time from performance.now in non-DOM environments [Scheduler] Get current time from performance.now in non-DOM environments Aug 5, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 5, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e35fbf0:

Sandbox Source
React Configuration

@sizebot
Copy link

sizebot commented Aug 5, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against e35fbf0

@sizebot
Copy link

sizebot commented Aug 5, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against e35fbf0

@@ -16,10 +16,24 @@ export let requestPaint;
export let getCurrentTime;
export let forceFrameRate;

const isWindowUndefined = typeof window === 'undefined';

const performance = isWindowUndefined ? global.performance : window.performance;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? Why not just use performance as a global in the first place?

Copy link
Contributor Author

@emilisb emilisb Aug 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is based on a comment in the previous code.

// Capture local references to native APIs, in case a polyfill overrides them.
const performance = window.performance;		
const Date = window.Date;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if all environments that don't have window are guaranteed to have global, or not. We don't already use global in this file yet.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example what if this code is loaded in a worker. Would it crash?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would feel better about refactoring to read globals directly from the global scope (without a qualifier) and aliasing them. Kind of like it was before 3f2cafe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thinking. I refactored the code. I had to refactor the browser test too, window is not considered as global under test environment.

@gaearon gaearon merged commit 5cff775 into facebook:master Aug 5, 2020
@gaearon
Copy link
Collaborator

gaearon commented Aug 5, 2020

Makes sense. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants