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

Expiration times #10426

Merged
merged 10 commits into from
Oct 10, 2017
Merged

Expiration times #10426

merged 10 commits into from
Oct 10, 2017

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Aug 9, 2017

An expiration time represents a time in the future by which an update should flush. The priority of the update is related to the difference between the current clock time and the expiration time. This has the effect of increasing the priority of updates as time progresses, to prevent starvation.

@acdlite acdlite changed the title [Work-in-progress] Assign expiration times to updates [Work-in-progress] Expiration times Aug 9, 2017
@acdlite acdlite mentioned this pull request Aug 10, 2017
17 tasks
@acdlite acdlite force-pushed the expiration branch 3 times, most recently from ee127ed to 796db96 Compare August 10, 2017 22:46
@acdlite
Copy link
Collaborator Author

acdlite commented Aug 11, 2017

Confirms that this makes the Triangle demo work as expected. Still need to write some unit tests, but this is ready for review.

@acdlite acdlite changed the title [Work-in-progress] Expiration times Expiration times Aug 11, 2017
@acdlite
Copy link
Collaborator Author

acdlite commented Aug 11, 2017

Added unit tests. I confirmed that all the existing tests, including the sync DOM ones, are passing when expiration is enabled.

@@ -249,6 +249,11 @@ var TestRenderer = ReactFiberReconciler({
useSyncScheduling: true,

getPublicInstance,

now(): number {
// Test renderer does not use expiration
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sebmarkbage I went back and forth on whether this should be an optional method. Landed on making it required because all renderers will need to account for features like expiration boundaries.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since scheduleDeferredCallback is required it seems reasonable that this would be too since they kind of pair together.

@@ -163,6 +163,22 @@ function shouldAutoFocusHostComponent(type: string, props: Props): boolean {
return false;
}

// TODO: Better polyfill
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this polyfill sufficient? Or am I overlooking something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's fine. Maybe we should colocate it with ReactDOMFrameScheduling to have all these in one place?

It is also performance.now aware: https://github.com/facebook/react/blob/master/src/renderers/shared/ReactDOMFrameScheduling.js#L73

@acdlite acdlite force-pushed the expiration branch 2 times, most recently from 67388a1 to 14e68e3 Compare August 11, 2017 22:08
window.performance &&
typeof window.performance.now === 'function'
) {
now = function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not now = performance.now and now = Date.now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

performance.now needs to be bound, but looks like Date.now doesn't. I'll update.

@@ -42,7 +42,7 @@ var {
Fragment,
} = ReactTypeOfWork;
var {Placement, Ref, Update} = ReactTypeOfSideEffect;
var {OffscreenPriority} = ReactPriorityLevel;
var {Never} = ReactFiberExpirationTime;
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that this name change decouples this expiration time from the UI context in which it's used. What we really mean is 'this might never finish updating, and that's ok' and that may or may not be literally "offscreen" in the UI.

TaskPriority,
HighPriority,
LowPriority,
OffscreenPriority,
Copy link
Contributor

Choose a reason for hiding this comment

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

haha, I guess it's not a rename after all. ("OffscreenPriority" to "Never" expirationTime)

@reactjs-bot
Copy link

reactjs-bot commented Sep 28, 2017

Deploy preview ready!

Built with commit 564c7e9

https://deploy-preview-10426--reactjs.netlify.com

@acdlite
Copy link
Collaborator Author

acdlite commented Sep 29, 2017

@gaearon I’ve rebased this locally, just haven’t pushed yet. I’ll work on getting all my PRs updated tomorrow.

This was referenced Nov 7, 2024
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.

6 participants