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

useEffect vs useLayoutEffect #87

Closed
mindplay-dk opened this issue Oct 21, 2019 · 7 comments
Closed

useEffect vs useLayoutEffect #87

mindplay-dk opened this issue Oct 21, 2019 · 7 comments

Comments

@mindplay-dk
Copy link
Contributor

Should we add support for useLayoutEffect?

From this article about the difference:

useEffect

...this runs after react renders your component and ensures that your effect callback does not block browser painting.

useLayoutEffect

This runs synchronously immediately after React has performed all DOM mutations
...
Your code runs immediately after the DOM has been updated, but
before the browser has had a chance to "paint" those changes (the user doesn't actually see the updates until after the browser has repainted).

From the React docs:

useLayoutEffect

The signature is identical to useEffect, but it fires synchronously after all DOM mutations. Updates scheduled inside useLayoutEffect will be flushed synchronously, before the browser has a chance to paint.

Also from the React docs:

Timing of effects

Unlike componentDidMount and componentDidUpdate, the function passed to useEffect fires after layout and paint, during a deferred event.

From my understanding, if you had an implementation of useLayoutEffect, adding useEffect could be as simple as this:

function useEffect(fn, deps) {
  useLayoutEffect(() => setTimeout(fn, 0), deps)
}

I did notice Preact has two separate implementations and dispatches them differently, so I'm not sure about this.

What do you think?

@yisar
Copy link
Collaborator

yisar commented Oct 21, 2019

It seems we can use setTimeout to simulate them for now.

@mindplay-dk
Copy link
Contributor Author

It seems we can use setTimeout to simulate them for now.

So your current implementation of useEffect actually works more like useLayoutEffect in React?

@yisar
Copy link
Collaborator

yisar commented Oct 22, 2019

Yes, It actually more like useLayoutEffect, but now #86 bothers me, It will take me more time to fix 😭

This was referenced Oct 26, 2019
@yisar
Copy link
Collaborator

yisar commented Dec 9, 2019

Hello, I have implemented useeffect and uselayout. Now I will explain the implementation and principle of it.

difference between them

useLayout will run sync, It will block the rendering
useEffect will run in requestAnimationFrame, It will execute before the next frame repaint.

role for them

[] => effect once, cleanup once util compenent removed
undefined => effect and cleanup everytime
[item] => effect and cleanup when the item changed

About tests

Maybe we only need to test useLayout now, because their difference is not reflected in the test environment.

I will reevaluate the test form PRs

@mindplay-dk
Copy link
Contributor Author

Any particular reason not to name it useLayoutEffect like in React?

Regarding the render-tests, I think you can roll back most of the changes you made to the test-conditions - all the tests were designed to work with what is now called useLayout, and testUpdates was designed to use that as well.

For the most part, you can simply replace every reference to useEffect in the test to useLayout, and all the tests should be working as intended.

We won't have any tests for what is now called useEffect then - and I don't know yet how we'll be able to test those, but we can work that out later. For now, I'd really like to see the tests we already had going green again. 😉

Once we're back in green, we can start working on tests for the new useEffect.

@yisar
Copy link
Collaborator

yisar commented Dec 9, 2019

I have rolled back most of the tests and let me debug them ~ just a few days ~ :laugh:

@yisar
Copy link
Collaborator

yisar commented Dec 24, 2019

We seem to have the useLayout test. I'll close it first.

@yisar yisar closed this as completed Dec 24, 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

No branches or pull requests

2 participants