Skip to content

Conversation

@acdlite
Copy link
Collaborator

@acdlite acdlite commented Nov 4, 2016

This is only observable in incremental mode.

@gaearon, I xit-ed some of your scheduling tests that use componentDidMount. I was going to rewrite them, but I don't think they test what their descriptions say they test. Let me know what you think and we'll either fix the tests or remove them. Resolved

@acdlite acdlite changed the title Updates from inside componentDidMount/Update should have Task priority [Fiber] Updates from inside componentDidMount/Update should have Task priority Nov 4, 2016
@acdlite acdlite assigned gaearon and unassigned acdlite Nov 4, 2016
@acdlite acdlite force-pushed the fibertasklifecycleupdates branch 2 times, most recently from dbb161f to ea083dc Compare November 4, 2016 21:39
class Foo extends React.Component {
componentDidMount() {
// Animation work that will get performed during animation callback
ReactNoop.performAnimationWork(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems to me that we still want a test that schedules animation work and deferred work in componentDidMount. These should not be synchronous since they've explicitly opted in to being async.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Please add a test that covers the case of animation/deferred work triggered from within a componentDidMount/componentDidUpdate.

@acdlite acdlite force-pushed the fibertasklifecycleupdates branch from ea083dc to d9d4ad6 Compare November 8, 2016 21:27
@acdlite
Copy link
Collaborator Author

acdlite commented Nov 8, 2016

Added tests

@acdlite acdlite merged commit c33230e into facebook:master Nov 8, 2016
acdlite added a commit that referenced this pull request Nov 8, 2016
acusti pushed a commit to brandcast/react that referenced this pull request Mar 15, 2017
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.

4 participants