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.

Sign up for GitHub

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

@wordpress/data: Introduce useSelect custom hook. #15737

Merged
merged 22 commits into from
May 27, 2019
Merged

@wordpress/data: Introduce useSelect custom hook. #15737

Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
edeec70
Add useAsyncMode hook
nerrad May 20, 2019
fbc9f0a
add useRegistry hook
nerrad May 20, 2019
b698a7e
add new useSelect hook
nerrad May 20, 2019
a331997
try new `withSelect` implementing useSelect internally.
nerrad May 20, 2019
dd58983
updates to exports
nerrad May 20, 2019
d781cfa
updates to README.md file
nerrad May 20, 2019
98bf61e
just use useLayoutEffect since GB as a whole doesn’t suppor server si…
nerrad May 20, 2019
4ae3461
only make `onStoreChange` dependent on registry and add dependency fo…
nerrad May 20, 2019
6aaeb56
latest changes
nerrad May 20, 2019
a06fa10
implement memoized mapSelect callback.
nerrad May 20, 2019
59bb4c3
implement pure on inner component
nerrad May 21, 2019
c761c54
implement try/catch approach for dealing with zombie props
nerrad May 21, 2019
86f864d
implement depdendencies argument
nerrad May 22, 2019
e891f53
hmm nope, let’s add _mapSelect to useMemo dependencies
nerrad May 22, 2019
e0aeac2
consider dependencies on whether to regenerate mapOutput
nerrad May 22, 2019
78d182b
and… useCallback to the rescue.
nerrad May 22, 2019
a757297
cover scenarios where state updated _during_ mount before subscriptio…
nerrad May 25, 2019
1de3ee6
update with-select tests
nerrad May 25, 2019
5565d5c
add new hook tests
nerrad May 25, 2019
71984ee
update tests
nerrad May 25, 2019
c65f45f
code cleanup and add documentation
nerrad May 25, 2019
cf2e824
update changelog
nerrad May 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
cover scenarios where state updated _during_ mount before subscriptio… 
…ns are set.
  • Loading branch information
@nerrad
nerrad committed May 25, 2019
commit a757297e3bca23a296b042f65ae14178d585d928
9 changes: 8 additions & 1 deletion packages/data/src/components/use-select/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,18 @@ export default function useSelect( _mapSelect, deps ) {
} catch ( error ) {
latestMapOutputError.current = error;
}

forceRender( {} );
}
};

// catch any possible state changes during mount before the subscription
// could be set.
if ( latestIsAsync.current ) {
renderQueue.add( queueContext, onStoreChange );
} else {
onStoreChange();
}

const unsubscribe = registry.subscribe( () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect this will break the re-rendering order, because the children will subcribe before the parents. Right?
I think the solution might be to subscribe synchronously at the render level (but only manage to do it once).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, see https://react-redux.js.org/next/api/hooks#stale-props-and-zombie-children

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm so the subscription callback would likely have to be a ref maybe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This basically means for me that subscriptions should happen at the root level of the hook and not inside a useEffect.

Something like

const registry = useRegistry();
const isAsync = useAsyncMode();
const previousRegistry = usePrevious( registry );
const previousIsAsync = usePrevious( isAsync );

if ( registry !== previousRegistry || isAsync !== previousIsAsync ) {
   // Trigger subscription
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://gist.github.com/markerikson/faac6ae4aca7b82a058e13216a7888ec

reduxjs/react-redux#1179

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think you'd be able to provide an example (codepen or something) where the React Async rendering would break this. Asking because that would be a good way to try and test alternative implementations addressing this issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would take a while to set up. The entire React API relies on render being idempotent. Note that the current implementation of withSelect also has this issue as it has side effects in the constructor.

From the React docs:

Conceptually, React does work in two phases:

  • The render phase determines what changes need to be made to e.g. the DOM. During this phase, React calls render and then compares the result to the previous render.
  • The commit phase is when React applies any changes. (In the case of React DOM, this is when React inserts, updates, and removes DOM nodes.) React also calls lifecycles like componentDidMount and componentDidUpdate during this phase.

The commit phase is usually very fast, but rendering can be slow. For this reason, the upcoming async mode (which is not enabled by default yet) breaks the rendering work into pieces, pausing and resuming the work to avoid blocking the browser. This means that React may invoke render phase lifecycles more than once before committing, or it may invoke them without committing at all (because of an error or a higher priority interruption).

Render phase lifecycles include the following class component methods:

  • constructor
  • componentWillMount
  • componentWillReceiveProps
  • componentWillUpdate
  • getDerivedStateFromProps
  • shouldComponentUpdate
  • render
  • setState updater functions (the first argument)

Because the above methods might be called more than once, it’s important that they do not contain side-effects. Ignoring this rule can lead to a variety of problems, including memory leaks and invalid application state. Unfortunately, it can be difficult to detect these problems as they can often be non-deterministic.

Strict mode can’t automatically detect side effects for you, but it can help you spot them by making them a little more deterministic. This is done by intentionally double-invoking the following methods:

  • Class component constructor method
  • The render method
  • setState updater functions (the first argument)
  • The static getDerivedStateFromProps lifecycle

Note:
This only applies to development mode. Lifecycles will not be double-invoked in production mode.

For example, consider the following code:

class TopLevelRoute extends React.Component {
  constructor(props) {
    super(props);

    SharedApplicationState.recordEvent('ExampleComponent');
  }
}

At first glance, this code might not seem problematic. But if SharedApplicationState.recordEvent is not idempotent, then instantiating this component multiple times could lead to invalid application state. This sort of subtle bug might not manifest during development, or it might do so inconsistently and so be overlooked.

By intentionally double-invoking methods like the component constructor, strict mode makes patterns like this easier to spot.

Copy link
Contributor

@youknowriad youknowriad May 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing. I think we probably have a lot of components that suffer from these issues. Makes me think we don't really support async mode. Not to say that we shouldn't but if feels like something we should invest in at a more global level.

From what I understand, say we have a global counter, we could increment (side effect) from within hooks. If the render function increments this counter, we'll still be certain that children components will pick a higher-number from this counter than their parent as even if react could double invoke parents render function ... it will still call the render of the children after the parents or is this a wrong assumption (assuming initial rendering)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the execution order:

render parent
render childA
render childB
useLayoutEffect cleanup childA
useLayoutEffect cleanup childB
useLayoutEffect cleanup parent
useLayoutEffect effect childA
useLayoutEffect effect childB
useLayoutEffect effect parent
useEffect cleanup childA
useEffect effect childA
useEffect cleanup childB
useEffect effect childB
useEffect cleanup parent
useEffect effect parent

You can't run side effects in a parent before running them in its children, because side effects only happen after the first render. This is true without hooks as well. The only alternative I see would be to stagger-render (pause render at each level of the tree to run side effects), but performance would suffer, and you would still have issues when children are rendered conditionally/dynamically.

if ( latestIsAsync.current ) {
renderQueue.add( queueContext, onStoreChange );
Expand Down