WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.47k stars 4.18k forks source link

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

Closed adamziel closed 3 years ago

adamziel commented 3 years ago

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:

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

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

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

youknowriad commented 3 years ago

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 commented 3 years ago

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

jsnajdr commented 3 years ago

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:

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 commented 3 years ago

@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 commented 3 years ago

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:


Things I disagree with in the comments above:

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:


Potential improvements I'm exploring:

It's is still very early but this is exactly what this draft PR is about https://github.com/WordPress/gutenberg/pull/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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)