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

Feature Request: Global didUpdate() #10900

Closed
ccorcos opened this issue Sep 27, 2017 · 19 comments
Closed

Feature Request: Global didUpdate() #10900

ccorcos opened this issue Sep 27, 2017 · 19 comments

Comments

@ccorcos
Copy link

ccorcos commented Sep 27, 2017

Currently, its not easy to write global logic that executes after React has re-rendered. The componentDidUpdate lifecycle method works great when your logic is isolated to a component, but I've found myself more and more recently wanting a global didUpdate hook baked into React.

A simple example where this is useful is if you want an isolated function (perhaps a keyboard shortcut) that creates an element on the screen and then focuses it.

const createNewTodo = async () => {
  const id = createTodo()
  await React.didUpdate()
  focusTodoItem(id)
}

At Notion, we've written custom logic for doing this, but it makes upgrading with React more difficult and unstable. I think this would be useful for others too, particularly those who use Redux and are building complicated UI interactions.

@aweary
Copy link
Contributor

aweary commented Sep 27, 2017

@ccorcos there is work being done on a top-level API for managing the render and commit phases. Check out this demo and explanation by @acdlite to get an idea of where things might be headed.

@gaearon
Copy link
Collaborator

gaearon commented Sep 27, 2017

Notion is pretty cool btw. Say hi to Surganov. :-)

@acdlite
Copy link
Collaborator

acdlite commented Sep 27, 2017

Interesting use case. Suggests that Redux is missing this feature, too. Something like dispatch(action, callback). Or a didUpdate argument to createStore.

@acdlite
Copy link
Collaborator

acdlite commented Sep 27, 2017

I think the thing the only reason you would need a global didUpdate is if focusTodoItem triggers many separate updates on many components, because you don't know which component to schedule the callback on. This happens to be how Redux works today; it calls setState on every connected component. But that's mostly an implementation detail. Conceptually, a dispatch is a single, atomic update, as if you were to call setState on the Provider, and the rest of the changes were propagated via context. (The only reason we don't do that is because context, uh, kinda sucks as-is :D). In this model, the didUpdate hook you're asking for would just be a setState callback on the Provider. Redux could expose it like dispatch(action, callback).

The reason you want the callback to scheduled per action is that, in the future, once we move to async, different actions may be scheduled with different priorities. So callbacks may fire at different times depending on the priority.

@acdlite
Copy link
Collaborator

acdlite commented Sep 27, 2017

Slight tangent: I have this idealized, possible-future version of React Redux in my head where there is no "store" and all state is stored as component state inside the Provider. Something like this:

class Provider extends React.Component {
  state = { storeState: null };
  dispatch = (action, callback) => {
    this.setState(state => {
      const nextStoreState = reducer(state.storeState, action);
      return {storeState: nextStoreState};
    }, callback);
  });
  //  ...
}

This would allow React's scheduler to abort, reorder, and rebase Redux actions according to their priority, which isn't possible today.

Though of course we'd have to figure out what this means for middleware and enhancers. And maybe fix context (though that's not necessarily a blocker, as this would still work with pub sub like we do today eh maybe not, would be hard to do pub sub in an async-friendly way).

@markerikson
Copy link
Contributor

Of course, then you don't really have "React-Redux" any more, you have "Redux in React" - which is not the same thing :)

@acdlite
Copy link
Collaborator

acdlite commented Sep 27, 2017

Don't you Redux-splain and React-splain at me, acemarke :)

@ccorcos
Copy link
Author

ccorcos commented Sep 28, 2017

Thanks guys!

@acdlite you touched on a few things there. The createNewTodo example I gave is the simplest possible example, but there are many more complicated examples that require this feature. Just looking through my application, we use it in 56 different places to do stuff like scroll things into view after they render, changing the user's selection after a user manipulation, focusing elements after they're dragged and dropped onscreen, clicking on elements after they're created, etc.

If I understand you correctly, what you were getting at is that the user's focus could be a state in redux and managed by the component itself. If you're living in Redux world, I can buy into that. But there are more complicated examples like clicking DOM elements and scrolling to elements after they appear that would be challenging to handle via state. These actions are one-off events and should probably live in the redux action.

Async renderering and reprioritizing is an interesting dilemma. There could be an API like didUpdate() which is called after the next render, and afterFlush() which is called when the render queue is empty.

@acdlite
Copy link
Collaborator

acdlite commented Sep 28, 2017

what you were getting at is that the user's focus could be a state in redux and managed by the component itself

No, that's not what I meant to suggest. The point of the callback is to fire right after the action has been flushed to the DOM, which is what the second parameter to setState provides: setState(update, callback). It's kind of like componentDidUpdate, but it only fires once that specific update has been rendered. Not on every render. This is important in a world where different updates have different priorities, because the next render might not include the update you just scheduled. By using the setState callback, you have a guarantee that the effect of the dispatched action has already been flushed to the DOM.

A future version of Redux could potentially expose this by adding a second argument to dispatch: dispatch(action, callback). Whether or not the focus state is stored in Redux isn't important.

@ccorcos
Copy link
Author

ccorcos commented Sep 28, 2017

Ahhh. I see what you mean. Right now, I'm just waiting for all the this.forceUpdate callbacks to complete (I'm using something similar to reactive-magic for state management). This means that the application might not be as responsive as it could but at least it works.

As far as implementing a granular callback with setState inside Redux, especially in a pure way, that sounds challenging because you don't know which components need to update until they begin to re-render...

I suppose that means the only way we can do this at a granular level is to implement our own render queues within the state management layer. But that doesn't mean we couldnt implement a global React.didUpdate that waits for all rendering to flush.

@acdlite
Copy link
Collaborator

acdlite commented Sep 28, 2017

you don't know which components need to update until they begin to re-render...

When you call dispatch, every component that needs to update in response should update in the same render pass. That isn’t always the case today, but that’s how it should work. And how we’ll need to make it work in the future so that Redux works with React’s async rendering.

A global didUpdate is pretty unlikely because it doesn’t scale. And besides, a setState callback on a root/Provider-type component, as described here, provides effectively the same thing.

@acdlite
Copy link
Collaborator

acdlite commented Sep 28, 2017

Want to make sure this part is clear: the only reason Redux triggers many setState calls for a single dispatch is to workaround the issue where context changes don’t propagate through a component whose shouldComponentUpdate returns false. That’s why each connected Redux component has its own subscriber to the store. But ideally, we’d only call setState on the Provider, and the connected components would update via context or some context-like mechanism. Regardless of how we implement it, you never want to have a situation where a single dispatch causes components to re-render at different times. That’s how you get “tearing,” or a momentarily inconsistent UI. It’s mostly not a big deal in today’s world because setState is synchronous (or part of a synchronous batch). So the tears aren’t noticeable. But in an async world, tearing is more of a problem, because it might be hundreds of milliseconds before an update is flushed, depending on what’s in the queue.

@markerikson
Copy link
Contributor

Y'know, that's an excellent point - I hadn't connected those dots yet.

Tagging @jimbolla and @timdorr as an FYI.

@timdorr
Copy link

timdorr commented Sep 28, 2017

@acdlite Has any research been done into coordinating setStates via explicit batches, especially in an async React render cycle? You would get that implicitly with priorities, where the grouping occurs along the priority values. But I think that would be particularly helpful with coordinating sections of a UI to avoid the inconsistencies of async actions.

More concretely, I'm thinking something like this:

handleChange = event => this.batchedSetState(BATCH_ID, { foo: event.target.value })

That would group up units of work and only reconcile with the DOM after the VDOM has been fully-rendered for those components.

That would be a way to ensure sync-like behavior when connect() updates a component.

BTW, I saw some library recently that does what you describe as a future react-redux. I forget the name, but it's basically that. I'll try to find it again...

@markerikson
Copy link
Contributor

Now I'm curious - if you've seen a lib, there's a good chance I've seen it, but nothing is immediately coming to mind.

@timdorr
Copy link

timdorr commented Sep 28, 2017

I only found freactal, but I'm pretty sure that's not it.

@ccorcos
Copy link
Author

ccorcos commented Sep 28, 2017

@acdlite I see your point abut tearing -- that makes sense. And we'll need some way to ensure that two components render in sync -- that's what was going on in that demo.

Pretty much every library that I've seen uses subscribers within components, otherwise it doesn't scale well. I could image libraries using custom render queues to enqueue forceUpdates and committing them all together. That way you could have multiple render queues and run animations on a separate queue that doesn't happen in sync. Cool stuff! Any ideas when this will land in production?

This is sounding more and more like an external library feature and less like a React feature the more we talk about it...

@acdlite
Copy link
Collaborator

acdlite commented Sep 28, 2017

@timdorr Yeah we've considered something like that. Probably wouldn't use batch IDs; as you say, batching in React is done by priority level. But we're also moving to model where the priorities are expressed as expiration times. The lower the priority, the later the expiration time; as the current time advances, things increase in priority and eventually "expire," at which point the rest of the work is flushed synchronously.

We group like-priority updates by rounding the expiration time. E.g. all low priority updates scheduled within ~30ms will have the same expiration time. But if you happen to start scheduling a whole bunch of updates right on the edge of a 30ms bucket, some could spill over into the next one. Doesn't even have to be a large amount of updates if you're right on the edge (say, 29.9ms).

So if you think about how Redux works today, after an action is dispatched and the store updates its state, we call setState on all the connected components. We need a way to guarantee that all those setStates receive the same expiration time. Maybe that's with an explicit batch API, maybe not. You don't want to encourage over-batching either, because you could get starvation: every time you add something new to the batch, it takes additional work before any of the updates in the batch can flush.

Another problem we'll have to solve with Redux is the lack of a commit phase, or synchronization phase. If someone calls getState, but the connected components haven't updated yet, the value they get from the store is not the value that is actually rendered on the screen. Tearing. So I think the getState API is inherently flawed in an async world.

This gets back to why we should store state in the Provider's component state and use setState updaters, as in my comment above. It allows us to leverage React to rebase actions.

An example. If you dispatch a normal priority action A, then a high priority action B, what we really want to do is flush twice: once with just B applied, but not A. Then later, with both A and B in that order:

const initialState = {log: []};

function reducer(state, action) {
  return {log: [...state.log, action.value]};
}

dispatch({value: 'A'}); // Normal pri
dispatch({value: 'B'}); // High pri

// First React flushes only B, because it has higher priority
// State should be {log: ['B']}

// Later, React flushes both A *and* B, in that order.
// State should be {log: ['A', 'B']}
// Note that we rebased the high pri action on top of the low pri action. Thanks, React!

But this means getting rid of "stores" as we know them today, along with things like getState. I don't think this means we're "replacing" Redux, @markerikson, because I don't think the store is the most important part of Redux. All your reducers and most middlewares can stay the same. Most patterns likely still work, just might need to change a bit. combineReducers is unchanged. And so on. Those are the essential parts of Redux, IMO, not the store.

Another way of saying this is that React is already a really good, sophisticated system for scheduling UI state updates. Redux is a pattern on top of that, not something that replaces it.

@gaearon
Copy link
Collaborator

gaearon commented Jan 6, 2018

We’re trying to funnel API proposals into our RFC repo so please feel free to create a pull request there and we’ll review it: https://github.com/reactjs/rfcs

I’ll close this one but we appreciate the discussion!

@gaearon gaearon closed this as completed Jan 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants