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

Bug: the order of effect and cleanup in Parent and Child component is weird #19482

Closed
xialvjun opened this issue Jul 29, 2020 · 12 comments
Closed
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@xialvjun
Copy link

xialvjun commented Jul 29, 2020

React version: v16.12.0

Steps To Reproduce

  1. just as the code example shows: just click the button 6 times respectively and see the console

Link to code example:
https://stackblitz.com/edit/react-effect-order-matters?file=index.js

The current behavior

useEffect logs: cpcc*1 + ppcc*n + pppc*1
useLayoutEffect logs: cpcp*(1+n) + cppc*1

The expected behavior

both useEffect and useLayoutEffect should log: cppc*(1+n+1)

Why I need a stable effect and cleanup order

There is a vanilla js package mapbox-gl whose use case is:

import mapboxgl from 'mapbox-gl';
var container = document.querySelector('#map');
var map = new mapboxgl.Map({ container });

map.on('load', function () {
  // every thing should be done after load
  map.addSource('route_source', route_source_data);
  map.addLayer({
    source: 'route_source',  // the layer relies on the source
    id: 'route_layer',
    ...other_layer_config,
  });
  // ...;
  do_many_things();
  // ...;
  // you must remove the layer before removing the source
  map.removeLayer('route_layer');
  map.removeSource('route_source');
});

Then I want to make a react version:

var vdom = (
  <Map opts={x}>
    <Source opts={xx}>
      <Layer opts={xxx}></Layer>
    </Source>
  </Map>
)

But since the order of react effect and cleanup in parent and child component is not stable (well, it' stable but it's weird), things became complex.

I think the effect and cleanup order matters because there are dependency relations between parent and child components.

And react should handle it.

There are some related(maybe) issues:
#16728
#17080

And the react lifecycle order is works right: https://stackblitz.com/edit/react-lifecycle-effect-order-right?file=index.js

@xialvjun xialvjun added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Jul 29, 2020
@xialvjun
Copy link
Author

@gaearon just like this comment, I filed a new issue. And in this issue, I gave a real use case, hoping this will draw you guys' attention.

@xialvjun
Copy link
Author

xialvjun commented Sep 4, 2020

Although react lifecycle order works right, it lacks componentDidUnmounted.

Related issue: #6424 . This issue is closed because

Closing as React's design is moving away from class-based lifecycle methods

But HookApi doesn't solve the problem of parent component can not clean up resource shared by its children.

@xialvjun
Copy link
Author

xialvjun commented Sep 9, 2020

To solve the problem of parent component can not clean up resource shared by its children, there is a hack way: #6424 (comment)

And by imitating the above method I wrote a hook which can express 6 lifecycles: willMount, didMount, willUpdate, didUpdate, willUnmount, didUnmount. And the parent children lifecycles running order is right: Parent willMount, Child willMount, Child didMount, Parent didMount, Parent willUpdate, Child willUpdate, Child didUpdate, Parent didUpdate, Parent willUnmount, Child willUnmount, Child didUnmount, Parent didUnmount, it's pccp*(1+n+1) which is cppc*(1+n+1)(this issue's useEffect expected behavior) transform to pc + cppc*(1+n+1) + cp

type life = "mount" | "update" | "unmount";
export const useWillDid = <T extends (life: life) => void | ((life: life) => any), D extends any[]>(
  fn: T,
  deps?: D,
) => {
  const inv = useRef<void | ((life: life) => any)>();
  const mounted = useRef(false);
  const is_last = useRef(false);
  is_last.current = false;
  inv.current = useMemo(() => {
    return fn(mounted.current ? "update" : "mount");
  }, deps);
  useEffect(() => {
    if (inv.current) {
      inv.current(mounted.current ? "update" : "mount");
    }
    mounted.current = true;
    is_last.current = true;
    return () => {
      if (is_last.current) {
        inv.current = fn("unmount");
      }
    };
  }, [inv.current]);
  const style = useMemo(() => ({ display: "none" }), []);
  const ref = useMemo(
    () => (ele: any) => {
      if (!ele && inv.current) {
        inv.current("unmount");
      }
    },
    [],
  );
  return <span ref={ref} style={style}></span>;
};

Even though I have wrote this hook which can solve my problem, I still thought It should be done by react. Hope you consider it more.

@mbest
Copy link

mbest commented Nov 20, 2020

What about using setTimeout in the parent's cleanup function?

@xialvjun
Copy link
Author

If useing setTimeout, what about parent's parent?

@mbest
Copy link

mbest commented Oct 17, 2021

If useing setTimeout, what about parent's parent?

True. It doesn't solve the problem well, since the setTimeout callbacks will run in the wrong order.

@derzwen
Copy link

derzwen commented Mar 3, 2022

Any news on this? I'm also using Mapbox and wanted to do the same thing and hit the same problem.

@tengl
Copy link

tengl commented Feb 23, 2023

We also have this problem. I've made a simple React app with maplibre to demonstrate the issue, and two possible work arounds. If someone has a better solution I'd be happy to hear it.

Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Apr 10, 2024
@xialvjun
Copy link
Author

bump

@github-actions github-actions bot removed the Resolution: Stale Automatically closed due to inactivity label Apr 11, 2024
Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Jul 13, 2024
Copy link

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

4 participants