[Concurrent] Safely disposing uncommitted objects

[danielkcz]

How to safely keep a reference to uncommitted objects and dispose of them on unmount?

For a MobX world, we are trying to prepare for the Concurrent mode. In short, there is a Reaction object being created to track for observables and it is stored within useRef.

The major problem is, that we can't just useEffect to create it in a safe way later. We need it to start tracking the observables on a first render otherwise we might miss some updates and cause inconsistent behavior.

We do have a semi-working solution, basically, a custom made garbage collector based on setTimeout. However, it's unreliable as it can accidentally dispose of Reactions that are actually being used but weren't committed yet.

Would love to hear we are overlooking some obvious solution there.

[gaearon]

@mweststrate

[danielkcz]

No need to ping him, he knows about it.

Either way, I wanted to ask for a more generic solution, not only related to a MobX. I can imagine similar problems will be happening all over the place once people start preparing for Concurrent. It feels like that React is not giving enough tools to tackle the problem gracefully.

[gaearon]

I’m tagging him precisely because “he knows about it” and because we’ve had conversations in the past about it. :-)

Given that he knows constraints of both I thought his input could be valuable.

As for your question — the design tradeoff we’re going for is that we don’t have a cleanup phase for renders that never committed. Rendering isn’t supposed to “allocate” resources that have to be cleaned up later. Instead we rely on the garbage collector. Resources that have to be disposed are only safe to allocate after the first commit.

I don’t know for certain if tradeoffs chosen by MobX are compatible or not with this approach, as I don’t know enough about MobX.

Can you explain what “reactions” are and why GC isn’t sufficient for them? Or why creating them can’t wait until after the first commit?

[danielkcz]

Can you explain what “reactions” are and why GC isn’t sufficient for them? Or why creating them can’t wait until after the first commit?

Well, I don't know exact internals how it works, it's very entangled there, but my assumption is that Reaction kinda attaches itself to observable variables present in the callback function. So as long as those variables exist, the Reaction won't be GCed. Those variables can definitely exist outside the scope of the component, that's kinda point of the MobX.

I think we have managed to figure out some way around it. Custom GC :) Basically, we globally keep the list of created reactions (in WeakMap) along with a time of their creation. If Reaction gets committed, all is fine and it gets removed from the list. A global timer takes care of cleaning up those that weren't committed in a given time.

I am wondering if you have some idea for a good "timeout" period? I mean time from render to commit. For now, we are thinking like 1 second. I assume it should be a long enough period even on a really slow computer. Is it bad idea to rely on that?

[danielkcz]

Or why creating them can’t wait until after the first commit?

@RoystonS wrote a good explanation of that. We could end up in very ugly situations ...

[urugator]

Can you explain what “reactions”

Mobx automatically manages subscriptions based on usage.
You define a function (reaction), which can access observable objects.
When the function is invoked, the observables accessed inside the function are added as dependencies to the function (reaction).
When you modify an observable, the function is automatically invoked again, resubscribing and optionally running a side effect.
We wrap render inside such function, so that if you access an observable in render, it creates a subscription:

const disposer = reaction(
  () => {    
    // accesses observables, creating subscriptions
    render()
  }, 
  () => {
    // a side effect to invalidate the component
    setState() // or forceUpdate() 
  }
)

why creating them can’t wait until after the first commit

The subscriptions are determined by rendering logic, so we would have to re-run the rendering logic in effect (double render).

why GC isn’t sufficient for them

While reactions come and go (together with react's component instances), the observable objects possibly stay for the whole app lifetime and they hold references to reactions (preventing them from being GCed).

[RoystonS]

Basically: we're trying to avoid double-rendering all MobX-observer components. The only time we actually need to double-render is if StrictMode/ConcurrentMode began rendering our component but then never got to the commit phase with us, or if something we're observing changed between initial render and commit. Taking a double-render hit on every component to cope with those (hopefully edge) cases is something we'd like to avoid.

[mweststrate]

@gaearon @urugator's description is pretty accurate. Whether it should be fixed through a React life-cycleish thing or by doing some additional administration on MobX side (timer + custom GC) have both it's merits. So let's leave that aside for now, and just focus on the use case:

Basic concepts

But the reason why immediate subscription is important to MobX (as opposed to a lifecycle hook that happens after render, such as useEffect) is because observables can be in 'hot' and 'cold' states (in RxJS terms). Suppose we have:

1. A collection of todo items
2. An expression (we call this a computed) that computes the amount of unfinished computed items
3. A component that renders the amount of unfinished computed items.

Now, the expression computed can be read in two different modes:

1. If the computed is cold, that is, no one is subscribed to it's result; it evaluates as a normal expression, returning and forgetting its result
2. If the computed is hot, it will start observing its own dependencies (the todo items), and caching the response as long as the todo item set is unaffected and the output of the computed expression is unchanged.

MobX without concurrent mode

With that I mind, how observer was working so far is as follows:

1. During the first (and any subsequent) render, MobX creates a so called reaction object which keeps track of all the dependencies being read during the render and subscribes to them. This caused the computed to become hot immediately, and a subscription to the computed to be set up. The computed in turn subscribes to the todos it used (and any other observable used). So a dependency graph forms.
2. Since there is an immediate subscription, it means that the reaction will no longer be GC-ed as long as the computed exists. Until the subscription is disposed. This is done automatically done during componentWillUnmount. This cause the reaction to release its subscriptions. And in turn, if nobody else depends on the 'computed' value, that will also release its subscriptions to the todos. Reaction objects can now be GC-ed.

So what is the problem?

In concurrent mode, phase 2, disposing the reaction, might no longer happen, as an initial render can happen without matching "unmount" phase. (For just suspense promise throwing this seems avoidable by using finally)

Possible solutions

Solution 1: Having an explicit hook that is called for never committed objects (e.g. useOnCancelledComponent(() => reaction.dispose())

Solution 2: Have global state that records all created reaction objects, and remove reactions belonging to committed components from that collection. On a set interval dispose any non-committed reactions. This is the current (WIP) work around, that requires some magic numbers, such as what is a good grace period? Also needs special handling for reactions that are cleaned up, but actually get committed after the grace period!

Solution 3: Only subscribe after the component has been committed. This roughly looks like;

1. During the first render, read the computed value cold, meaning it will be untracked
2. After commit (using useEffect) render another time, this time with proper tracking. This serves two goals:
3. The dependencies can be established
4. Any updates to the computed between first rendering and commit will be picked up. (I can imagine this is a generic problem with subscribing to event emitters as part of useEffect, updates might have been missed?)

The downsides:

1. Every component will render twice!
2. The computed will always compute twice, as there is first a 'cold', which won't cache, and then a 'hot' read. Since in complicated applications there might be entire trees of depending computable values be needed, this problem might be worse than it sounds; as the dependencies of a computed value also go 'cold' if the computed value itself goes cold.

Hope that explains the problem succinctly! If not, some pictures will be added :).

[gaearon]

Does MobX have bigger problems in Concurrent Mode? How valuable is it to fix this in isolation without addressing e.g. rebasing?

[mweststrate]

I think the later problem can be largely avoided by making a clear distinction between component own state (use useState instead of MobX) and state that lives outside the component tree (external domain state). If I am not mistaken, my intuition is that rebasing only makes sense for 'volatile ui state' such as the state of pending requests, switching tabs / pages, charts etc. But state that is domain specific (e.g. a collection of todo's), typically doesn't need rebasing as that would lead to weird behavior for the end-user?

…

On Wed, Apr 10, 2019 at 4:36 PM Dan Abramov ***@***.***> wrote: Does MobX have bigger problems in Concurrent Mode? How valuable is it to fix this in isolation without addressing e.g. rebasing? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15317 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhOMREb3lbrEL4q9sccIXfNj5f5vfks5vffcHgaJpZM4ccUAP> .

[NE-SmallTown]

Any picture? Thanks~ :)

[radex]

We have a very similar issue to MobX's with 🍉 WatermelonDB, but even more tricky because of asynchronicity.

When connecting the data source to components, we subscribe to the Observable. This has to happen directly in render, because:

• if we subscribe in an effect/on did mount, we'll ALWAYS render twice, and the first render will be wasted, even if we have the data ready to render and we can skip a bunch of unnecessary work.
• it wouldn't be appropriate for an useObservable hook to return null on initial render, because it's not always clear what the component developer is supposed to do with it. For example, if an observable value is necessary to fetch another observable value, the developer would have to make a whole bunch of extra logic just to get to the return null (since we can't just bail early and render null with hooks - like we can today with a withObservables HOC).
• it would be appropriate for useObservable to throw a Promise and suspend, however, we risk showing stale data (which can lead to bugs or ugly glitches) if we treat the data source like an async promise, not a reactive Observable — it could be that between the time the initial data is fetched, and component re-rendered, the value of the observable has changed again. But it's not possible to know that on the second render if we don't stay subscribed to it. Which means we have to subscribe before we throw a Promise — and have to deal with cleaning up the subscription if the component never ends up being rendered

[CaptainN]

We have a similar situation in Meteor, where we set up a computation and its observers the first time it's run. If we use useEffect we have to either wait for useEffect to do that, causing a render of no content, or use useLayoutEffect without touching the dom, which would violate the suggestions and complicate the API for our users and still render twice if somewhat synchronously, or render twice with useEffect - once without setting up observers which is hacky (or doing a full create/dispose lifecycle) and then always at least one additional time after useEffect (this was janky in testing).

What I ended up doing is something based on setTimeout, but it takes a lot of juggling (we also have some backward compat code which complicates our implementation. It has to set up the computation and a timeout, then in useEffect it has to look for a few things; Is the computation still alive (did setTimeout trigger dispose)? If not rebuild it, and re-render (back to 2 renders). Did we get a reactive change between render and useEffect? If so, re-run the computation, and re-render (this is fine, because it should anyway).

I originally set the timeout to 50ms, assuming commit would happen relatively quickly, but in testing in concurrent mode 50ms missed almost all commits. It often took longer than 500ms to commit. I've now set the timeout to 1000ms. I also saw that there are a lot of discarded renders for every commit, between 2-4 to 1. (I have no idea how to write tests for all this, this is based on some counter/console logging.)

Since there is a lengthy gap between render and commit, I had to decide what to do for reactive changes that happened in renders which will never be committed. I could either run the user's reactive function for every change, knowing it might be run by 4 different handlers without doing anything, or block reactive updates until the component was committed. I chose the latter.

I tried a workaround using useLayoutEffect. I incorrectly assumed it would run in the same event loop as render. If it had, the code for juggling when and if to build/rebuild/re-run/re-render could all be greatly simplified, and a timeout of 0ms or 1ms would work just fine. In Strict Mode, this worked beautifully (and greatly simplified our implementation). In practice, in concurrent mode, useEffect and useLayoutEffect performed no differently from each other.

BTW, one additional thing we found challenging was trying to leverage a deps compare. We had at one point copied some parts of react out of core to manage that, before just switching to useMemo to use its deps compare without it's memoization. I understand that may lead some some additional re-creations, our old HOC used to do that anyway (and it does that now if a user supplies no deps to our useTracker hook), so I went with useMemo just to reduce our overhead - but it'd be nice if the deps compare functions were somehow available to use.

[CaptainN]

A quick additional observation - one of the things I have in place is a check to make sure that if the timeout period elapsed (currently 1 second), it will make sure to restart our computation when/if the render is eventually committed in useEffect. This has almost never triggered outside of concurrent mode (and it rarely triggers inside concurrent mode at 1second). It did trigger today during a particularly janky local dev reload. All of this would be so much simpler if there was a way to reliably clean up after discarded renders without resorting to timeout based GC hacks.

[aleclarson]

Using dual-phased subscriptions (phase 1: collect observed object/key pairs and their current values, phase 2: subscribe to them), you can avoid any timer-based GC and postpone subscribing until React commits. In phase 2, be sure to check for changed values before subscribing, so you can know if an immediate re-render is required. This is what I did with the Auto class in wana. Of course, this only applies to observable objects. Streams and event emitters, not so much.

[CaptainN]

We'd have to modify a bunch of stuff in Meteor core to convert that to a dual-phase subscription model like that (although we can fake it by just setting up the subscription, running the computation, then disposing of it immediately, to restart it again in useEffect). An additional problem is that the return data from a Meteor computation is not immutable, and can be any type of data (with possibly very deep structures), so a deep compare would be necessary to make sure data hasn't changed, or we'd have to render twice (same problem as before).

[aleclarson]

We'd have to modify a bunch of stuff in Meteor core to convert that to a dual-phase subscription model like that

The issue with Meteor is that its tracking system doesn't associate "observable dependencies" with state. In other words, the Tracker.Dependency instance is disconnected from what it represents. Although, you could get around that by updating a nonce property on the dependency whenever it emits a change event.

(although we can fake it by just setting up the subscription, running the computation, then disposing of it immediately, to restart it again in useEffect)

Wouldn't you miss changes between render and commit if you did it that way?

An additional problem is that the return data from a Meteor computation is not immutable, and can be any type of data (with possibly very deep structures), so a deep compare would be necessary to make sure data hasn't changed, or we'd have to render twice (same problem as before).

Yep, that's the problem with not using observable proxies that track every property access.

[aleclarson]

Another good use case for useCancelledEffect is being able to lazily allocate shared state (which is stored in a global cache or in context) during render, without the risk of leaks or the need to implement a timer-based GC.

[CaptainN]

Wouldn't you miss changes between render and commit if you did it that way?

Yes. It's actually that way now even with the timer. If an update comes in before the render is committed (which takes about a half a second in my testing), I had to make a choice - run the user's reactive function for every uncommitted computation, or don't run any of them until the render is committed (I detect if an update happened, but don't run user code until later) - I chose the latter. In the example I gave in the other comment, I'm saying we could simply always rerun the computation and user function again in useEffect - but then we have to always eat a second render, and all the rest.

Another good use case for useCancelledEffect is being able to lazily allocate shared state (which is stored in a global cache or in context) during render, without the risk of leaks or the need to implement a timer-based GC.

I had thought it would be useful if we could get some kind of deterministic ID for whatever the current leaf is, which is consistent for all renders of current component, even those that would not be committed. Then I could use my own global store. I think someone explained that couldn't happen though, for various reasons. (Can react-cache be used for this? I'm not clear on what that is useful for.) I could offload the naming/ID responsibility to the user, but that seem onerous, and I'm not sure how I'd be able to detect collisions - though, would I have to? 🤔

[aleclarson]

This problem also exists without Concurrent mode enabled, because no-op state changes can trigger a render that bails out before effects are ever called. Source

[kitten]

We've had to find a solution to this for urql. It's a similar problem to what MobX has in that we're using streams that start a GraphQL operation when subscribed to which then yields either synchronous or asynchronous results (cached or fetching).

The solution to this can be found here: https://github.com/kitten/react-wonka/blob/master/src/index.ts
We're essentially dealing with 1) synchronous subscriptions since they may immediately return a result, so they can't be started in useEffect only, 2) cleanups on interrupted or double renders, 3) updates to values in useEffect.

I'll leave some simplified / generic code here in case someone else has to implement it from scratch, since we haven't found a generic solution yet that could be shared (like an alternative to use-subscription for instance)

Interruptible Subscriptions implementation walkthrough

On mount we start the subscription if it hasn't started yet:
(note: most of this is pseudo code since the full implementation is linked above)

const useObservable = observable => {
  const subscription = useRef({
    value: init,
    onValue: value => {
      subscription.current.value = value;
    },
    teardown: null,
    task: null,
  });

  if (subscription.current.teardown === null) {
    // Suppose we have a subscription that needs to be started somehow, with a teardown:
    subscription.current.teardown = observable.subscribe(value => {
      subscription.current.onValue(value)
    });
  }

  return subscription.value;
};

Then we schedule an immedite teardown/cancellation of the subscription using scheduler (this way we get really predictable and cooperative timing):

import {
  unstable_scheduleCallback,
  unstable_cancelCallback,
  unstable_getCurrentPriorityLevel,
} from 'scheduler';

const useObservable = observable => {
  // ...
  if (subscription.current.teardown === null) {
    subscription.current.teardown = observable.subscribe(value => {
      subscription.current.onValue(value)
    });

    subscription.task = unstable_scheduleCallback(
      unstable_getCurrentPriorityLevel(),
      subscription.teardown
    );
  }

  return subscription.value;
};

On interrupted rendering we won't have any effects run (useEffect and useLayoutEffect) but on a normal render we don't want to cancel the subscription by calling the teardown. We can use useLayoutEffect to cancel the teardown (unstable_cancelCallback) since that has synchronous timing (after mount during the commit phase)

const useObservable = observable => {
  // ...
  if (subscription.current.teardown === null) {
    subscription.current.teardown = observable.subscribe(value => {
      subscription.current.onValue(value)
    });

    subscription.task = unstable_scheduleCallback(
      unstable_getCurrentPriorityLevel(),
      subscription.teardown
    );
  }

  useLayoutEffect(() => {
    // Cancel the scheduled teardown
    if (subscription.current.task !== null) {
      unstable_cancelCallback(subscription.current.task);
    }

	// We also add the teardown for unmounting
    return () => {
      if (subscription.current.teardown !== null) {
        subscription.current.teardown();
      }
    };
  }, []);

  return subscription.value;
};

Lastly we'd like a normal useEffect to take over for normal cooperative effect scheduling. In the real implementation we also send updates back to a subject which affects the observable, so we do that there as well. For simplification/generalisation purposes that's excluded here though.

const useObservable = observable => {
  // ...

  // We add an updater that causes React to rerender
  // There's of course different ways to do this
  const [, setValue] = useReducer((x, value)) => {
    subscription.current.value = value;
    return x + 1;
  }, 0);

  if (subscription.current.teardown === null) {
    // ...
  }

  useLayoutEffect(() => {
    // ...
  }, []);

  useEffect(() => {
	// In useEffect we "adopt" the subscription by swapping out the
    // onValue callback with the setValue call from useReducer above
    subscription.current.onValue = setValue;

    // If we've cancelled the subscription due to a very long interrupt,
    // we restart it here, which may give us a synchronous update again,
    // ensuring that we don't miss any changes to our value
    if (subscription.current.teardown === null) {
      subscription.teardown = observable.subscribe(value => {
        subscription.onValue(value)
      });
    }

    // We'd typically also change dependencies and update a Subject,
    // but that's missing in this example 
  }, []);

  return subscription.value;
};

Lastly we need to update this to work on server-side rendering.
For that we swap out useLayoutEffect with a useIsomorphicEffect

const isServerSide = typeof window === 'undefined';
const useIsomorphicEffect = !isServerSide ? useLayoutEffect : useEffect;

And add some code that immediately cancels the subscription on the server-side:

const useObservable = observable => {
  // ...
  if (subscription.current.teardown === null) {
    subscription.teardown = observable.subscribe(value => {
      subscription.onValue(value)
    });

    if (isServerSide) {
      // On SSR we immediately cancel the subscription so we're only using
      // synchronous initial values from it
	  subscription.current.teardown();
    } else {
      subscription.current.task = unstable_scheduleCallback(
        unstable_getCurrentPriorityLevel(),
        subscription.current.teardown
      );
    }
  }
  // ...
};

Afterwards we added some code that handles our updates to an input value.

tl;dr We start the subscription on mount synchronously, schedule the teardown using scheduler, cancel the scheduled teardown in useLayoutEffect (since that tells us that effects will run and we're not in an interrupted render), let a useEffect adopt the subscription and switch an onValue updater over to a useReducer update.

If note is that this only works as well as it does for us because we're already handling starting operations only once if necessary and avoiding duplicates, so in a lot of cases our subscriptions are already idempotent. I can totally see that this isn't the case for everyone, so a lot of subscriptions with heavy computations won't be as "safe" to run twice in rapid succession.

This is a little inconvenient to say the least. I'd love if either use-subscription or a built-in primitive could introduce a concept of observing changing values that starts a subscription synchronously but also ensures it's cleaned up properly on interrupts or unmounts. 🤔

[mweststrate]

@FredyC At a quick glance, the above might be cleaner than the current approach in lite 2. Thoughts? (Probably better to discuss further in the repo, just wanted to cc you in!)

[trusktr]

That's all fair enough. It's React team's library, and React team has the right to say how React is supposed to be used and does not need to support everyone else's alternative paradigms.

---

That being said,

It is not intended as a constructor

That may be the intent, but by providing ways to define method-like things (f.e. useEffect) the FC body has become similar to a constructor because the "instance" (the lexical scope) has the ability to have long lived things associated with it after creation (normally those things are created in something like useEffect, not in the function body itself) including DOM objects.

Actually, the "instance" is the cumulative set of scopes from every time that the FC has been called in its life time (a very important but subtle difference, sometimes a confusing one).

The similarity of FCs to classes may be an unintentional invitation for code that creates state at the beginning. The render (f.e. the FC return value) is the only thing that an external dependency-tracking system can rely on for tracking dependencies used for rendering.

---

Something like onCancel (or whatever name it would have) seems like a very easy way to solve this problem.

[lxsmnsyc]

To solve this, what I did is to treat my own objects to have a dispose/cleanup logic, and created a hook similar to what @dai-shi created:

function useDispose(dispose) {
  // schedule a cleanup, 16 could be a good number.
  const timeout = setTimeout(dispose, 64);
  // cancel the scheduled cleanup if the side-effects went through.
  useEffect(() => {
    clearTimeout(timeout);
  }, [timeout]);
}

This worked, in my case, specially just to prevent an external source (e.g. an object that subscribes to multiple events) from subscribing twice.

[Andarist]

Actually, the "instance" is the cumulative set of scopes from every time that the FC has been called in its lifetime (a very important but subtle difference, sometimes a confusing one).

This mental model is not correct - some render calls can be freely dropped by React so it's a false assumption to think about an instance being a cumulative set of scopes from every time that FC has been called in its lifetime. If anything it's a cumulative set of committed scopes. This distinction is very important in understanding how to obey React rules.

It has been mentioned here in the thread already - but those rules are old. They just start to be way more important now but breaking those rules probably has caused mem leaks and similar problems in the past already.

I can't say about the community as a whole but it has always been quite clear for me that constructor in a React's class was not an appropriate place for side-effects and while it was maybe annoying at times I just have always used such logic to componentDidMount and usually had no problem with that. It was a simple refactor.

[bvaughn]

Adding a simple hook (f.e. onCancelled) would be fairly simple for React to implement (I think) and it would prevent a lot of complicated code in code written on top of React (namely those state tracking libs). It's a win-win.

You are right that it would be easy to add such a hook. The reason we haven't done it is that it would have a significant negative impact on performance.

I think of the canonical reason why a render might get thrown away as the following:

• because there's no way to prevent a React.FC from returning something, so basically they always "render" because they always construct a vdom tree. Otherwise, with an idea like useRender in my last comment, React could selectively skip renders for any of the reasons you listed (saving CPU and memory cost, and avoiding the above problem in a different way).

Unless I'm reading this comment wrong, I think it's a misconception. A component can already return null to say that it does not want to render anything. That is not the reason a render may be thrown away. There are a few reasons why React might not commit the value rendered by a component, including:

• Something else scheduled a higher priority update (an interruption)
• Another component threw an error
• Another component suspends to wait on loading data
• The pre-rendering animation use case Dan mentioned above

[kitten]

@bvaughn @gaearon Are you considering a system for push-based subscriptions that coöperate with the component lifecycle by any chance? I still find it to be a little odd that most systems are rather pull-based once they instantiate their state, i.e. outside of effects we prefer to be able to get either "snapshots" (useMutableSource) or the state directly (useSubscription). The same goes for suspense, it's all rather dependent on having pull-based state/effect sources.

I think a lot of problems could be solved if there was a way to make push-based systems work better together with React's hooks & lifecycles. This would indeed be rather novel, but I think it'd fix this. This is especially important for derived event sources, where the state isn't pull-accessible. Currently in urql we work around this by creating a state snapshot by subscribing & unsubscribing to our source to get a current one-time snapshot of our state.

However, if state was tied to some kind of observable source from the start of a component lifecycle, similar to how useMutableSource prevents tearing and forces the source into a more coöperative timing, it'd have a good method of ensuring that a synchronous event would lead to a valid initial state and that subsequent events would have a sufficient amount of backpressure applied to them — in other words, a push-event would still act just like a normal state setter today.

[CaptainN]

If there are truly use cases that aren't solvable with the current primitives that React provides, then the community should collect a list of specific use cases that are completely incapable of being solved so that we can figure out what primitives are needed.

Here's the issue in Meteor. Meteor uses a computation for setting up observers on "reactive" sources, based on accesses to those sources on the first run of the computation function. It does that in the render pass now, with some caveats. We could simply turn off the observer for the render pass, then run it again in useEffect - in this case it would always need to run again, because state might change between render and useEffect. We currently use timers to try and avoid that, but we already had to disabled that for one case to support StrictMode, and it's proving unmaintainable.

The question is, if we have to run our setup code a second time on commit, is it really earning performance, or just moving the performance hit off to other parts of the app? Then again, in most cases in Meteor, it's probably not a big deal to run the computation again - most computations are relatively cheap, unless there is some huge complex mini mongo query or something

I went down the rabbit hole of trying to pull that out in to a data source, as described in the "suspense for data" section in the React docs, but that has a number of problems. Primarily, it complicates what is otherwise a clear and easy API for us (with useTracker - except for deps, developers regularly mess up deps), but it also felt like I was just picking up that kicked down the road performance work, and things were getting COMPLICATED. Ultimately, I couldn't figure out a real great way to make an externalized API like that work because of various scoping issues, and the complexity of basically rebuilding redux with a sort of dependency injected inversion of control system (which is where I was headed) felt like a step backwards.

[markerikson]

@CaptainN : fwiw, that "might need to run again because state could have changed before the effect" part sounds exactly like how the new useMutableSource hook is intended to work. Have you looked at that at all?

[arcanis]

I've also hit a few times the case of a component needing to instantiate objects tied to the WASM heap. In those cases there is no garbage collection to rely on for freeing uncommitted resources (not yet, at the very least), so the only choices seem to be to enter a world of double-renders with useLayoutEffect, or to break the CM assumptions. I'm currently doing the first, but it seems prone to degraded performances should those components be nested a bit too much.

[edit] I made a search on "WASM" before sharing my experience, but I think the following comment is pretty much what the React team would answer to that: #15317 (comment)

That said, you could just rely on GC eventually. The WeakRef proposal lets you add some cleanup logic. Its timing would not be deterministic, but neither would it be for a similar React-managed mechanism. So this seems like the longer term solution for the rare cases where you can’t use an effect and for some reason have to allocate a resource during render.

Unfortunately it'll be years before WeakRef reaches general availability. I guess it can work if CM takes years too 😛

[CaptainN]

@markerikson I did look at that a few months back, but wasn't sure exactly what that does or how (or if it was ready for use) - is that finalized or still in RFC? Last time I looked at it, I thought it would drop any component using it in to a "deopt" mode, opting out of the benefits of concurrent mode for that component, and wanted to see if I could keep the benefits.

It's on my list to pursue a useMutableSource based solution for a future version the generic useTracker (or simply eat the second setup pass) while moving to a new more tailored set of hooks to integrate React with Meteor, which can be implemented in the pure way, without a ton of complexity. Those hooks already look better.

[markerikson]

@CaptainN : useMutableSource has been merged in, but I think it's still only available in "experimental" builds. The "deopt" behavior as I understand it only kicks in in certain situations.

Not saying it's going to magically fix your problems, but I think you should go review the RFC and PR discussions around it and give it a shot to see if it does potentially address your use case.

[brucou]

But the React component contract has always emphasized that the render method is meant to be a pure function of props and state.

@gaearon Functions components that use hooks are not pure in the most general cases (i.e., unless you find a hook that is actually pure). I am not sure what is the render method you are referring to. I assume you are referring to the render method use in React's class API? That render method should indeed be a pure function of the parameters it is passed, as it is documented. But then there seems to be some confusion in your next assertion:

But it shouldn’t be surprising that when we add new higher-level features to React that rely on the component contract having a pure render method, those features may not work. I think it’s fair to say that render being pure is well-documented and one of the very first things people learn about React.

The component contract you are referring to is the class component contract. If you get rid of class components, you get rid of that contract. Then function components (if they use hooks) renounced to purity so you can't really refer there to a pure render here. Given that functional components and concurrent mode are a new thing, why not clarify the new contracts for the new thing? That takes possibly wrong assumptions out of the way together with the bugs they create. This especially applies to concurrent mode as it seriously changes what was assumed to be a contract in former React versions. Some people continue to think that it is React as usual apart from cosmetic changes but it is really a fundamental change. I am not sure this has been captured yet in the documentation.

[bvaughn]

I am not sure what is the render method you are referring to.

class ClassComponent extends React.Component() {
  // ...

  render() {
    // This is a render method and should be pure.
  }
}

function FunctionComponent(props) {
  // This is also a render method and should be pure.

  // Built-in hooks (e.g. React.useState) are an exception
  // because they are explicitly managed by React
  // and so are more like locally created variables.
}

[benjamingr]

Can't MobX just use a FinalizationRegistry @mweststrate ?

[brucou]

@bvaughn The second case (FunctionComponent) is a function. Methods usually refer to functions within classes (or by extension objects). Then, again, it is not accurate to say that function components should be pure when them being impure is far from being an edge case (due to hooks usage). There is no such thing like an impure function that is like a pure function except this or that. People get confused precisely because we use the same words to mean different things. In the context of computer science, functions that may be impure are usually referred to as procedures. So you may call a function component a render procedure if you will, but not a pure function in a large majority of cases (when they use hooks). Which is one of my points and the reason why people get confused by functional programming notions decaffeinated with exception and metaphors.

[bvaughn]

I'm not looking to debate the semantics of "methods" or "functions", just addressing your comment that you were "not sure what is the render method". The answer is: Dan was referring to both class components and function components.

[gaearon]

I hear that the documentation does not do a good enough job defining “purity”. This is something we’re keeping in mind for the upcoming docs rewrite.

On the other hand, if you take the technical argument that “function components are impure because of Hooks”, I don’t think you can claim that “the class render method is pure” either. After all, it reads this.props and this.state which are by definition properties on a mutable instance — thereby making any render method technically impure. This technicality hasn’t prevented people from understanding that the rendering process in React (whether with a class method or a function component) is meant to be generally pure. But we could definitely be stricter in our definitions and this is good feedback.

[brucou]

@Gaeron you are indeed right. The render class method is often impure too. That escaped me. I thought it received props as an argument, but it is true that it gets it from this. Actually often the reason why you would use a class is precisely to do this kind of impure things that pure functions does not let you do (this.state). Your point is well taken.

But still, whatever it is that you mean by the rendering process is meant to be generally pure can probably be expressed in a more precise (albeit more wordy) way. Did you mean something like the vDom should be (generally) computed only from props, context and children --- independently of how you actually get that information, and that effectful components should be the exception and non-effectful components the rule? There is a pattern like that, which combines renderless components and pure components. Or did you simply mean that React functions components can do all kind of things as long as the computation of those things only depends on props, context, children etc and a variety of React APIs? That is, the output (understood as effect executed + computed return value) of function components depend only on inputs and the React runtime state? So basically you don't do effects that do not go through a React API.

With concurrent mode, I believe that accuracy is going to be even more important. The hard part is going to have developers build the mental model that shortens debugging sessions and reduce the need for technical support. Of course, examples of usage will help a ton and you have good stuff already on the documentation site. But having consistency in words and meaning (we call that ubiquitous language in TDD) goes a long way too. It was for instance important that React introduced different words for rendering and for committing in order to explain the different operations that React does at different moments and it would be unfortunate to mix one with the other randomly.

[mweststrate]

Random update: in mobx-react-lite we are now experimenting with using a FinalizationRegistery as suggested (and executed) by @Bnaya / @benjamingr. So far it looks quite promising and something that could be abstracted away in a generic hook (possibly with a timer based fallback).

The two liner summary of the approach is that in the component some a local object is allocated and stored using useState: const [objectRetainedByReact] = React.useState({}). With a finalizationRegistery we can then register a callback to get a notification when the objectRetainedByReact is garbage collected by the JS engine, which should be a safe heuristic that a an instance wasn't committed.

[dai-shi]

Interesting. So, it doesn't even check if it's committed with useEffect. Should we expect JS engine not to garbage collect objects very soon if there's enough free memory?

[mweststrate]

It does useEffect to mark comitted, left all the implementation details out 😉

…

On Tue, 3 Nov 2020, 22:31 Daishi Kato, ***@***.***> wrote: Interesting. So, it doesn't even check if it's committed with useEffect. Should we expect JS engine not to garbage collect objects very soon if there's enough free memory? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15317 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBHHYJZDCAVPTSM4DBDSOCAEPANCNFSM4HDRIAHQ> .

[benjamingr]

Just a small correction, I suggested using FinalizationRegistry (for MobX) - the execution is 100% @Bnaya :]

(the idea came after discussing the idea ±2 years ago for MobX in some JSConf with Daniel Erhenberg in the context of "how should Node behave" and I figured "what other libraries am I involved with and may use this")

Michel had an interesting idea (to make this into a generic hook - useDisposer or something) which I believe would be very cool.

@gaearon I think it would be very useful if you can't commit to cleanup or when cleanup runs if React provided this sort of hook as part of React since the use case is pretty common. Is that something you'd be willing to explore or is that antithetical to the direction you want to see React take?

In any case I think it's better to discuss this once @Bnaya is further along with the useDisposer work.

[dai-shi]

Cool. I was going to draft something like useDisposer with FinalizationRegistry, but will sit and wait. My two cents is this is still like an escape hatch and is not something like a first recommended pattern.

[Andarist]

Is using FinalizationRegistry really a good idea? I mean - I understand some part of the frustration coming from people wanting this kind of a feature, I also understand why this is not something that the React team wants to support (unless compelling arguments are found). However, resorting to things like this is a huge bandaid, a hack. I think that no one even would question that. So if this is a hack (and not a small one) - is it worth pursuing? I feel like either idiomatic React approaches should be used - which for some use cases are at least quirky to deal with, I don't have super compelling use cases for myself regarding this as, usually, I can rather easily work around this or convince the React team that this is really needed for the community. It's obvious that the React team understands challenges and the use cases mentioned here but those deviate from their model and they are hesitant to implement this as it can actually be a footgun if not used properly, but maybe it's time to gather the people most interested in this in sort-of a focus group that would figure out the path forward? Maybe this happens somewhere in the background - I don't know, but at this point in time this really seems like a stagnant issue. A pretty much the same thing is repeated all over the place from both sides and the discussion seems kinda fruitless.

If no consensus is found then I expect such hacks to be actually implemented and used, not because I feel like that they are needed that much but just because of human nature. We all have strong opinions about things and it's inevitable that something like this will exist in the community eventually, question is - can we prevent it? Or at least eliminate it from being needed for more use cases?

Given the recent work on the docs - some of the use cases mentioned here should probably be documented there to present what's the React team's stance on them, what are proposed solutions.

[markerikson]

@Andarist fwiw, my take is that this might be a "hack" in the sense of "well, this really isn't how you ought to be writing your code in the first place based on the rules", but it may very well be a valid technical solution for MobX's use case and potentially other folks as well.

Given that, I wouldn't expect the React team to specifically recommend this approach, but perhaps it can be published as an independent package by whoever's writing it, and the React team could maybe at least document it as a known workaround.

[sebmarkbage]

Especially as an incremental upgrade approach can be a valid strategy. As new research is going into new patterns.

However, there's one interesting case to think about. The main motivation for FinalizationRegistry to be added to the language now of all times, is because it allows building interop with WASM linear memory (aka array buffers). In a more general sense, it's how you build interop with a tracing GC language and a ref count or manual collection GC language.

For any library implementing its backing store in a WASM world, this would be the primary way to interop with it.

There are many patterns that might be a bad idea around this (like opening web socket connections without deduplication until the GC runs). The core principle of using FinalizationRegistry to interop with an external memory management library isn't though. That's the point of FinalizationRegistry in the first place.

[Bnaya]

I've created useDisposeUncommitted hook, similar to MobX's impl
https://www.npmjs.com/package/use-dispose-uncommitted
I've also opened a self-PR to collect feedback on the initial version and api
https://github.com/Bnaya/use-dispose-uncommitted/pull/3/files

[danielearwicker]

And for anyone like me who ends up here because their jest test uses observer and logs:

Jest did not exit one second after the test run has completed.

This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with --detectOpenHandles to troubleshoot this issue.

Try:

import { clearTimers } from "mobx-react-lite";

// at end of your test:
clearTimers();

This tells the timer-based cleanup to run immediately instead of waiting N seconds.