react-navigation / rfcs

RFCs for changes to React Navigation
Other
88 stars 16 forks source link

devTools prop #86

Closed Ashoat closed 4 years ago

Ashoat commented 4 years ago

Motivation

Many dev tools and debugging libraries try to track "actions" or "events" that occur in an app.

In my case, in production I track events so that I can report them in the case of a crash. This makes it easier to understand what led to a crash, and oftentimes I can glean repro steps just by looking at the log.

In dev mode, my app reports these events directly to a debugging console. Reactotron is a good option for this, especially now that it can integrated into Flipper. Redux Dev Tools can also be used for this sort of tracking functionality, similar to how it is done in ReactNav today.

Implementation

Before ReactNav5 it was possible to rely on the onNavigationStateChange prop for this. But in ReactNav5, the onStateChange prop only reports the new navigation state, without any identifying action. This makes it hard to understand an event at-a-glance.

Because state changes in ReactNav5 don't necessarily derive from actions, matching them back up to actions is something that needs to be done post-hoc. The logic for doing this for the existing Redux Dev Tools integration currently resides in the useDevTools hook.

I'd like to extract that functionality to a public API so users don't need to reimplement it.

API

We'll probably want to expose a prop on NavigationContainer.

It's important to avoid giving the user the wrong ideas about this API. Because of the prevalence of the Redux action/reducer/state model, it's easy for the user to assume that all state changes are a function of the actions. If they take this assumption on faith, and implement business logic or persistence (for instance) based on that assumption, they will have a bad time.

For that reason it's probably best to name this prop something like devTools. We could allow the user to pass in an object that contains various callbacks that ReactNav could trigger:

  1. trackAction - triggered whenever an action is dispatched. Passed the action object and the new state†
  2. trackStateChange - triggered whenever the state changes. Passed the new state, along with a list of actions that have occurred since the last state change. This list may be empty, for instance in the case of hydrating a stale navigation state, or in the case of the user dynamically changing route definitions

I'm not tied to this particular API - open to any comments & suggestions!

† Right now, if the user calls resetRoot on the NavigationContainer ref, this passes a string action to useDevTools (@@RESET_ROOT). This feels fairly nonstandard, and would probably be best to avoid in an external API. If we want to keep this we could add an onResetRoot callback to the devTools prop

ericvicenti commented 4 years ago

Hey Ashoat! I really like the idea of cleaning up useDevTools to use a public API rather than the internally-hardcoded global.__REDUX_DEVTOOLS_EXTENSION__.

Good point that state changes do not map 1-1 to actions. But, I think people might want to track actions for other reasons than devTools, so maybe we should just add an onAction prop to the container?

My other concern is that devTools.trackStateChange seems very similar to the existing onStateChange prop.

We could possibly provide a default behavior to onStateChange and onAction which calls out to the redux devtools. There are other ways to sugar-coat this, including the devTools prop you described.

I think you're on the right track, I just hope to keep the container props as simple as possible.

Ashoat commented 4 years ago

That makes sense. I'm curious to hear @satya164's thoughts, as he initially suggested explicitly mentioning "dev tools" in the naming to avoid any confusion.

I'm pretty flexible on the API, mostly just want a way for users of the library to track actions.

janicduplessis commented 4 years ago

onAction would be very useful, I was actually trying to implement preloading requests with relay (starting the request as soon as the navigation action is dispatched instead of waiting for the screen to render) but it was pretty hard and required a custom router.

satya164 commented 4 years ago

I think people might want to track actions for other reasons than devTools

Actually the reason I talked to @Ashoat about a devtools specific API is because I don't want people to be tracking actions other than for debugging purposes. It can cause subtle issues and coupling when people start to track actions imo.

For example:

Instead, people should rely on checking what changed in the state, e.g. - the focused screen changed, so change statusbar style or something. I think this also works like React works, run an effect based on what changed, not how it changed.

onAction would be very useful, I was actually trying to implement preloading requests with relay (starting the request as soon as the navigation action is dispatched instead of waiting for the screen to render) but it was pretty hard and required a custom router.

Isn't action -> screen rendering almost instant? At least for network requests, a few milliseconds doesn't seem like it would be noticeable.

Anyway, the use case seems to be to get notified before screen render, not necessarily get notified of actions. An action might not even change anything. With suspense, I assume we'd need some sort of API for preloading anyway, so it's something we'd need to think about.

Another case to know before state change is to be able to prevent a state change, e.g. prevent going back from the screen (e.g. if user is editing something), but it also doesn't need to be an action listener (rather can't be, goBack and pop should both work for this for example).

For this use case, there would need to be some API that'd need to notify with upcoming state before it's applied, with ability to prevent it, or some more specific APIs for specific purposes.

If separate devtools API isn't something we want, to avoid duplication, another option is to expose something like containerRef.addListener('__UNSAFE__action', callback) to emphasize that this is unsafe.

Then the devtools stuff can be implemented in a hook and used like:

useReduxDevToolsExtension(containerRef)

The container's ref has other methods like getRootState, resetRoot, dispatch etc. for the devtools to interact with the navigator's state.

Ashoat commented 4 years ago

Closing now that @satya164 has implemented this in @react-navigation/devtools!