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

Replace hand-picked redux concepts with react hooks...? #26849

Closed
adamziel opened this issue Nov 10, 2020 · 11 comments · Fixed by #26866
Closed

Replace hand-picked redux concepts with react hooks...? #26849

adamziel opened this issue Nov 10, 2020 · 11 comments · Fixed by #26866
Labels
[Package] Core data /packages/core-data [Package] Data Controls /packages/data-controls [Package] Data /packages/data [Type] Discussion For issues that are high-level and not yet ready to implement.

Comments

@adamziel
Copy link
Contributor

adamziel commented Nov 10, 2020

Could we do away with most redux concepts that we use today, and instead rely on React hooks?

This is an audacious proposal directly building up on findings from 26692. Not much thought given to cost/benefit analysis yet but the amount of work involved is potentially huge. Even if this ends up not being pursued, having a discussion would be beneficial anyway. Let's talk!

Our redux actions and selectors have complex needs such as invoking side-effects or actions and selectors from other stores. To enable these interactions, we have concepts like:

  • Registry selectors
  • yield anotherAction()
  • Async actions, controls, thunk middleware

At the same time, we do all this things in React components using:

  • useSelect
  • useDispatch
  • useEffect and promises

I wonder - what are the advantages of using the former over the latter? These ideas seem to be interchangeable, only using both means more code paths to maintain plus twice the learning and a higher barrier of entry. Let’s see how React hooks could replace the existing concepts side by side:

Actions with yield

These two seem to be equivalent and interchangeable:

export function* undo() {
	const undoEdit = yield controls.select( 'core', 'getUndoEdit' );
	if ( ! undoEdit ) {
		return;
	}
	yield {
		type: 'EDIT_ENTITY_RECORD',
		...undoEdit,
		meta: {
			isUndo: true,
		},
	};
}
export function useUndo() {
    const undoEdit = useSelect( select => select('core').getUndoEdit() );
    const { editEntityRecord } = useDispatch( 'core' );
    const undo = useCallback(() => {
        if( ! undoEdit ) {
            return;
        }
        editEntityRecord( {
            ...undoEdit,
            meta: {
                isUndo: true
            }
        } );
    }, [ undoEdit ]);
    return undo;
}

Only with the latter one doesn’t need to know about generators, controls, or dispatching actions inline.

Controls

Similarly, controls seems to exist just to handle the async behavior and grant access to different stores. Consider the yield controls.select( 'core', 'getUndoEdit' ); part of the undo action above. This is the underlying select control:

// controls.js:
export const builtinControls = {
	[ SELECT ]: createRegistryControl(
		( registry ) => ( { storeKey, selectorName, args } ) =>
			registry.select( storeKey )[ selectorName ]( ...args )
	),
  // ...
}

With hooks, this control wouldn’t be required at all - its role would be fulfilled by useSelect(). The same goes for controls.dispatch.

Async controls

How about async controls, for example apiFetch? I find this:

export function* deleteEntityRecord( kind, name, recordId, query ) {
	/* ... */
	deletedRecord = yield apiFetch( {
		path,
		method: ‚DELETE’,
	} );
	/* ... */

To be the same as this:

export function useDeleteEntityRecord() {
	/* ... */
	return useCallback(async ( kind, name, recordId, query ) => {
		/* ... */
		await apiFetch( {
			path,
			method: ‚DELETE’,
		} );
		/* ... */
	});
}

In this case yield turned out to really be just a reimplementation of await - we „hack” generators to simulate async behavior. Actions invoke other actions that are really controls, a middleware (?) calls the control handler, and then the handler waits for the actual promise. Why not remove the indirection and await for the promise directly?

Registry selectors

This registry control:

// component.js:
const postContent = useSelect(
	( select ) => select( 'core/editor' ).getEditedPostContent(),
	[]
);

// selectors.js:
export const getEditedPostContent = createRegistrySelector(
	( select ) => ( state ) => {
		const postId = getCurrentPostId( state );
		const postType = getCurrentPostType( state );
		const record = select( 'core' ).getEditedEntityRecord(
			'postType',
			postType,
			postId
		);
		// ...
	}
);

Seems to be just this hook in disguise:

// component.js:
const postContent = useEditedPostContent();

// hooks.js
export const useEditedPostContent = () => {
	const { postId, postType, record } = useSelect( select => {
		const postId = select('core/editor').getCurrentPostId();
		const postType = select('core/editor').getCurrentPostType();
		const record = select( 'core' ).getEditedEntityRecord(
			'postType',
			postType,
			postId
		);
		return { postId, postType, record };
	});
	// ...
} );

Resolvers

Resolvers are particularly interesting. I didn’t quite figure out all the code paths that are running in order to make them work just yet, but it seems like this controls plays an important role:

[ RESOLVE_SELECT ]: createRegistryControl(
	( registry ) => ( { storeKey, selectorName, args } ) => {
		const method = registry.select( storeKey )[ selectorName ]
			.hasResolver
			? '__experimentalResolveSelect'
			: 'select';
		return registry[ method ]( storeKey )[ selectorName ]( ...args );
	}
)

I imagine that without controls, a canonical useSelect hook could be used to explicitly trigger the resolution when needed.

Public API

One downside of react hooks is that one could no longer call wp.data.select( 'x' ).myFunc() as easily. Or is it true? This isn't fully fleshed out yet, but I imagine exploring "bridge layer" could yield a solution:

const PublicApiCompat = memo(() => {
    useSelect(select => window.wp.data.select = select);
})

Other considerations

  • @wordpress/redux-routine would go away entirely.
  • At least some redux middlewares would go away.
  • Concepts of async actions, controls, registry selectors would all go away.
  • All that’s left would be pure reducers, pure selectors, and pure action creators. Other than that, the logic would live in hooks.
  • The code would be more approachable by new contributors.
  • Using hooks would improve debugging. Currently with async generators stack traces aren’t always informative plus stepping through the code is challenging.
  • A nice side-effect of this would be that it would remove the concept of registry selectors and thus make store-specific selectors possible.
  • It would break BC big time, unless of course we can build a „compatibility layer” to keep the existing API in place but really call the new one under the hood.

Anything else?

Am I missing anything? Is there anything that makes this a blunder or a technical impossibility? Really curious to learn what does everyone think. cc @noisysocks @draganescu @youknowriad @mtias @talldan @tellthemachines @gziolo @nerrad @jorgefilipecosta @ellatrix @kevin940726 @azaozz @aduth @nosolosw

@adamziel adamziel added [Package] Data /packages/data [Package] Core data /packages/core-data [Package] Data Controls /packages/data-controls labels Nov 10, 2020
@youknowriad
Copy link
Contributor

I think there's some missing context here. One of the important aspect of @wordpress/data is that it's react agnostic. You can retrieve and interact with that in any JS context?

Another thing missing is the "store creator" and "data consumer" are two different persons in general.

The store consumer needs to define the public APIs of its stores (actions and selectors) and the data consumer only uses useSelect and useDispatch to communicate with this public API (he doesn't need to know controls, resolvers..., that's implementation detail)

How do you imagine a store creator defining his resolvers with that in mind?

@ockham
Copy link
Contributor

ockham commented Nov 10, 2020

cc/ @jsnajdr b/c I hear you like @wordpress/data 😜

@jsnajdr
Copy link
Member

jsnajdr commented Nov 11, 2020

I agree that the generator-based routines and controls built on top of the rungen and redux-routine libraries add a lot of extra complexity that is not entirely justified. You need to learn many new concepts and rules that you've likely never seen before and that you'll never use again outside the Gutenberg world.

The concepts in these libraries are almost identical to redux-saga, another generator-based side-effect engine for Redux.

I think it's not a coincidence that these libraries were both created around January 2016, at a time when async/await was a little-known experimental syntax, promises were a rarely used new thing, and most JS programmers worked with Node-style callbacks (and often descended into callback hell). A generator-based async engine must have been something refreshing and pleasant to use.

Today, the most natural thing to do would be to write async functions. And in connection with Redux, dispatch and call them as thunks.

Because what is the difference between a generator control function and a standard async function? The main difference is that generators use the Command pattern to do things. When a generator calls select or apiFetch:

function *myControl() {
  const param = yield select( 'my/store', 'getParam', 1 );
  const response = yield apiFetch( '/rest/is/best', { param } );
  return response.body;
}

then the select and apiFetch don't actually do any selecting or fetching. They merely return a command object:

{ type: '@@data/select', selector: 'getParam', params: [ 1 ] }
{ type: 'apiFetch', path: '/rest/is/best', query: { param: 1 } }

Then it's up to the rungen engine and its registered controls to interpret these commands.

There is a layer of indirection between the author of the generator function and the actual execution. Do we need that extra indirection? I'd say that in vast majority of cases, we don't need it at all. The only exception I can think of is maybe apiFetch, which we might want to reimplement transparently, to fetch data from some custom store rather than from REST API over HTTP.

In Redux thunk language, slightly modified for @wordpress/data, the control would look like:

import apiFetch from '@wordpress/api-fetch';

const myControl = () => async ( { select, dispatch } ) => {
  const param = select( 'my/store' ).getParam( 1 );
  const response = await apiFetch( '/rest/is/best', { param } );
  return response.body;
}

Such a control/thunk can then be dispatched to store either directly:

store.dispatch( myControl() );

or using the React bindings:

const dispatch = useDispatch();
dispatch( myControl() );

The dispatching process makes sure that the thunk is called with the right select and dispatch params.

In conclusion, I agree that the generators could be entirely phased out without losing much.

React bindings
But I don't agree that the replacement are hooks, at least not the most low-level, canonical one. @wordpress/data should continue to be React agnostic, with its own API. And there should be optional React bindings for it. Just like you can use Redux without React, or the Apollo GraphQL client without the React hooks.

Then you can use @wordpress/data in code that's vanilla JavaScript or that uses any other UI library.

Fetching data
As an aside, the way how @wordpress/data exposes APIs for fetching and caching data, and for modifying them over network, is, I'm afraid, outdated. It uses strange jargon like selector, dispatch, resolve, invalidateSelector that doesn't map well to the mental model of querying, fetching, caching and re-fetching data.

While the primary concern is to fetch data from some resource, @wordpress/data is "store-first", i.e., the primary concept is the cache, which is quite a minute implementation detail.

More modern and ergonomic libraries have been developed lately, like react-query or swr. Here, the programmer provides information about:

  • some identification of the data I want (the query)
  • some recipe how to actually fetch them
  • some policy how I want to cache the data (memory, local storage, no cache) and when to invalidate the cache and refetch the data (on component mount, every 10 seconds, when certain mutations are executed, never)

One thing I don't need to implement is the cache and its set and get methods.

I think we should use these libraries as inspiration. I believe it can be implemented as a thin query layer over existing @wordpress/data stores, with easy and full backward compatibility.

"Real" application state
There are data stores like core/block-editor that don't do any network data fetching, but instead maintain some complex data structure in memory. That's a place where Redux really shines. The data structure is immutable, updated in a controlled and tractable way with reducers and actions... Now start using the structure at many distinct places in the UI, and require it to be shared, and Redux becomes even more useful.

The "manage complex state in memory" and "fetch data from network and cache them" use cases are very different from each other. They could be very easily implemented with two completely different libraries. And if you use the latest "Autumn 2020 edition" of React best practices and libraries, they are. It's a bit unfortunate that we use the same @wordpress/data for both, as it makes thinking about the library more difficult.

To conclude, I think removing the redux-routine generators and replacing them with something else, and implementing a new query layer are rather orthogonal, independent tasks. I don't see them having much in common except both making @wordpress/data easier to use.

@kevin940726
Copy link
Member

@jsnajdr Just want to state that this comment pretty much sums up all my feelings about @wordpress/data. 10000% agree! Mostly where @wordpress/data or its usage is outdated, and there are way better ways to do these kinds of things now.

@youknowriad
Copy link
Contributor

I realize that I didn't share my sentiment earlier, so I wanted to add to the discussion here and what I think of the current state of @wordpress/data and where it should go:

  • I entirely agree with you all when you say that creating async stores with resolvers + controls + actions + selectors + reducer is very hard. Luckily we don't have much stores like that (just one), but it's definitely something that I hope we can improve.

  • One thing I think we can improve as well is performance: Redux (whether you subscribe to just one store or multiple) doesn't scale very well. If you keep rendering a growing list of components on a page (say a block list), these components will add computation time regardless of whether they rerender or not. In a block list for instance, no matter if the components of that blocklist subscribe to just a single store say 'core/block-editor', for each change of any block, they'll still have to call the selectors that power these components and that's way too much. We did introduce the async mode which "solves" this issue or more exactly "hides" this issue but I believe we can do better there.

  • Dependencies and relying on "strings" for store names is a minor thing but also something that I don't like and which @gziolo has a decent proposal for.


Things I disagree with in the comments above:

  • Using hooks to solve the "store creation" issue is a no-go for me because of the react dependency and also because it moves more responsibilities to components authors where store makers are better equipped to make the right decisions about their data.

  • More modern and ergonomic libraries have been developed lately, like react-query or swr: I've used these libraries a couple times in some side projects. They have indeed very great DevX to start with, and are very simple to use for "REST API fetching" (or any kind of fetching) capabilities but I don't think they provide a good solution for us.

these libraries shine where you're in a "read-heavy" environment and when you're a very limited number of mutations. While that's true for a lot of applications, our main one is very different from that: the editor. We need a layer that fetches data, allow edits to data, support dirty checking, and undo/redo ... from completely unrelated components. This is something these solutions are just not good for.

It doesn't mean we can get inspired from some of their implementation details to provide utilities for things like cache management....


Things I think the data module is very good at:

  • Extensibility: selectors and actions are a decent enough abstraction that allows devs to access and manipulate the data in the exact same way, no matter how the data is organized by the store maker. That's a huge point for plugins developpers (components authors basically)...

  • I think useSelect and useDispatch are decent APIs: while some of the work in the js community right now tend to merge these two together useAtom or useSWR... it's mostly cosmetic if you have a centralized store definition.

  • One of the great aspects of the data module (which is I think not very well known) is that with registerGenericStore you can register stores that are not redux based, you can have your own custom implementation and it should just work. This is also very important as it allows us to make experiments to provide better utilities and tools to handle different kinds of stores and solve some of the concerns I raised above.


Potential improvements I'm exploring:

  • the performance factor is an important one, I'd like for our applications to scale properly when we render more and more components. This can be solved by making sure components only "subscribe" to the data they're interested in. Tools like mobx, recoil and the like solve this by relying on observables basically (atoms are just hidden observables).

  • Recoil and jotai also have very interesting ways to define async behavior and data which can be used as a replacement for selectors + resolvers + controls + async actions. Reducing the number of concepts would make it easier to create these kinds of stores. It's a very different way of defining stores though and a mental model that requires some getting used to in the beginning but probably simpler than what we have anyway.

It's is still very early but this is exactly what this draft PR is about #26866 (don't look at the code much, it's still very experimental) but the interesting thing is that it implements exactly the ideas I share above while retaining backward compatibility and as you can see by the "performance" numbers on the PRs, it is still more performant than master even if I didn't refactor the store definitions (because it considers a store as an atom, useSelect discovers and subscribes to the stores it needs and not all stores)

@adamziel
Copy link
Contributor Author

adamziel commented Nov 12, 2020

Excellent discussion! I agree hooks may not be the best tool for the job. What matters is that we find out what is. #26866 looks promising, I'm curious to see how it will unfold. Atom-based stores have the potential to remove a ton of crust from the mental model and the codebase. Plus, as Riad mentioned, components would only get notified about the specific updates they're interested in. 🤞

@jsnajdr
Copy link
Member

jsnajdr commented Nov 12, 2020

I entirely agree with you all when you say that creating async stores with resolvers + controls + actions + selectors + reducer is very hard.

@youknowriad What is your sentiment about continuing to use rungen and redux-routine? Is it worth the extra complexity and learning curve?

these libraries shine where you're in a "read-heavy" environment and when you're a very limited number of mutations. While that's true for a lot of applications, our main one is very different from that: the editor.

Let me offer a different perspective: the actual block editor and its interaction with REST API, the differential edits, dirty checking, autosaves, is and will always be an unique exception. Not amenable to any pre-packaged framework-y solution.

Just about everything else, even in Gutenberg, like working with post types, categories, block directory etc., is different: it's mostly reading lists of things, optionally filtered, sorted and paginated, and performing basic CRUD operations on them.

Expanding further to wp-admin (or Calypso as a good blueprint for a wp-admin-ish app), we can see even more of REST CRUD with sorting, filtering and pagination that is very simple and un-sophisticated. For all that, a fetching library with awesome devex would be very valuable. And it doesn't hurt much if it doesn't scale beyond certain complexity.

@adamziel
Copy link
Contributor Author

@youknowriad What is your sentiment about continuing to use rungen and redux-routine? Is it worth the extra complexity and learning curve?

@jsnajdr I was considering starting a PR to explore removing these dependencies, but then I took a sneak peek of the code in #26866 and it seems like it could replace redux entirely, I believe this includes rungen and redux-routine as well.

@adamziel
Copy link
Contributor Author

Let me offer a different perspective: the actual block editor and its interaction with REST API, the differential edits, dirty checking, autosaves, is and will always be an unique exception. Not amenable to any pre-packaged framework-y solution.
Just about everything else, even in Gutenberg, like working with post types, categories, block directory etc., is different: it's mostly reading lists of things, optionally filtered, sorted and paginated, and performing basic CRUD operations on them.

@jsnajdr are you hinting at using two different lines of libraries for these two use-cases? As in the specialized solution for the block editor, and a react-query-like library for other parts of the system?

@jsnajdr
Copy link
Member

jsnajdr commented Nov 12, 2020

are you hinting at using two different lines of libraries for these two use-cases?

It could be more like using two layers, low-level and high-level, of the same stack. Most components would use the high-level, react-query-like API, and other components can build their own specialized REST/state engine from lower-level primitives.

It's like writing UI in React vs manipulating DOM directly. Very often we do both at the same time, in the same component. And when done well, both approaches complement each other rather than being in conflict.

@youknowriad
Copy link
Contributor

@youknowriad What is your sentiment about continuing to use rungen and redux-routine? Is it worth the extra complexity and learning curve?

just like @adamziel shared, I think these are essential building blocks on how the current async stores works, so we might not be able to remove this any time soon (third-party usage) but it doesn't mean we shouldn't explore simpler ways to do that. My PR (atomic stores) replaces all of that: redux + rungen + redux-routine with one low level library (called @wordpress/stan for now). If it proves to be successful, we could deprecate one in favor of the other.

Let me offer a different perspective: the actual block editor and its interaction with REST API, the differential edits, dirty checking, autosaves, is and will always be an unique exception. Not amenable to any pre-packaged framework-y solution.

I agree that the block editor is an exception but it's our main project here so this needs solving anyway. I agree that useSWR and the like have great DevX for component authors but I believe right now our devxp for creating the stores is bad right now but for consuming them, it's not that bad. Writing useSelect + useDispatch even if it adds some indirection to something more straightforward as { isLoading, refresh, data, error } = useQuery( somequery ), it's not a difference that is too big and I believe for the sake of consistency, we should just accept it for now. I don't mind explorations to see if/how we can improve these (useAtom is a possibility for instance) but I'd personally like to solve the most urgent issues IMO (performance at scale and creating stores devx)

@gziolo gziolo added the [Type] Discussion For issues that are high-level and not yet ready to implement. label Nov 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Core data /packages/core-data [Package] Data Controls /packages/data-controls [Package] Data /packages/data [Type] Discussion For issues that are high-level and not yet ready to implement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants