Task: Migrate wp-data stores from registerStore to createReduxStore

noahtallen commented 1 year ago

Details

The main reason we should do this is that registerStore is deprecated. But the main benefit we get is the new approach to using stores gives us much better typescript integration. Previously, one would have to declare types to override the entire wp data package.

This is actually impossible to do now that the package provides its own typedefs, so we've had to rely on the temporary solution of casting to selector types like (select( 'automattic/foo' ) as FooSelectors).selector(). This is very clunky, and we also lost type info for dispatch calls. (See this comment for more info.)

As a result, we should migrate stores to use the "new" format (it's been available for a long time actually, though it wasn't supported by the WordPress versions we support at first.) And ideally, also start using stores via imports instead of via string names!

This will restore type data we lost in the upgrade, and also make types much easier to work with for WordPress data.

This is all made possible by our recent WordPress package updates done in #73890.

Checklist

A store is registered in each of these files. See PRs for example migrations.

Editing Toolkit Plugin

[x] editing-toolkit-plugin/global-styles/src/store.js @noahtallen #74394
[x] apps/editing-toolkit/editing-toolkit-plugin/whats-new/src/store.js @noahtallen #74395
[x] apps/editing-toolkit/editing-toolkit-plugin/wpcom-block-editor-nav-sidebar/src/store.ts @noahtallen #74396
[x] apps/editing-toolkit/editing-toolkit-plugin/starter-page-templates/store.ts @noahtallen #74346

Calypso data-stores

[x] Update documentation about how to use stores, which references the previous register approach. @noahtallen #74602
[x] packages/data-stores/src/analyzer/index.ts @tyxla #74414
[x] packages/data-stores/src/auth/index.ts @tyxla #74418
[ ] packages/data-stores/src/automated-transfer-eligibility/index.ts @noahtallen #74550
[ ] packages/data-stores/src/domain-suggestions/index.ts @noahtallen #74551
[x] packages/data-stores/src/i18n/index.ts @tyxla #74427
[ ] packages/data-stores/src/plans/index.ts
[x] packages/data-stores/src/products-list/index.ts @tyxla #74566
[x] packages/data-stores/src/reader/index.ts @tyxla #74425
[x] packages/data-stores/src/subscriber/index.ts @flootr #74430
[ ] packages/data-stores/src/user/index.ts
[x] packages/data-stores/src/wpcom-features/index.ts @tyxla #74473
[x] packages/data-stores/src/wpcom-plans-ui/index.ts @tyxla #74468
[ ] Update documentation to remove legacy .register instructions once all stores are migrated.
[ ] Update docs to include info about when to use react query instead?

Payments related

[x] client/state/partner-portal/credit-card-form/index.ts @tyxla #74656
[x] client/my-sites/checkout/composite-checkout/hooks/wpcom-store.ts https://github.com/Automattic/wp-calypso/pull/74452
[ ] client/my-sites/checkout/composite-checkout/payment-methods/netbanking.tsx
[ ] client/my-sites/checkout/composite-checkout/payment-methods/paypal.tsx
[ ] client/my-sites/checkout/composite-checkout/payment-methods/credit-card/store.ts
[ ] client/my-sites/checkout/composite-checkout/payment-methods/wechat/index.tsx
[ ] packages/wpcom-checkout/src/payment-methods/alipay.tsx
[ ] packages/wpcom-checkout/src/payment-methods/bancontact.tsx
[ ] packages/wpcom-checkout/src/payment-methods/eps.tsx
[ ] packages/wpcom-checkout/src/payment-methods/giropay.tsx
[ ] packages/wpcom-checkout/src/payment-methods/ideal.tsx
[ ] packages/wpcom-checkout/src/payment-methods/p24.tsx
[ ] packages/wpcom-checkout/src/payment-methods/sofort.tsx

On hold:

These two persist a single option to help keep a modal close. Could probably be migrated to wp preferences.

[ ] apps/editing-toolkit/editing-toolkit-plugin/wpcom-block-editor-nux/src/store.js @noahtallen #74454
[ ] apps/editing-toolkit/editing-toolkit-plugin/wpcom-global-styles/store.js @noahtallen #74500

These data stores persist several options. Since these persist much more of the store, it'd be harder to migrate them to wp preferences.

[ ] packages/data-stores/src/help-center/index.ts
[x] packages/data-stores/src/launch/index.ts @tyxla #75269
[ ] packages/data-stores/src/onboard/index.ts
[ ] packages/data-stores/src/site/index.ts
[ ] packages/data-stores/src/stepper-internal/index.ts

Address this after double registration issue is solved:

[ ] Follow-up task: once the double-registration problem is solved, simplify how ETK imports stores across different bundles. (currently it uses string-based store keys)

tyxla commented 1 year ago

Thanks for opening an issue, @noahtallen!

I and the rest of @Automattic/team-calypso-frameworks would love to aid with those.

Perhaps @sirbrillig and the rest of @Automattic/shilling folks could aid with the checkout-related ones?

sirbrillig commented 1 year ago

I'm curious: in my testing, while passing the store to useSelect does correctly provide types, passing the store to useDispatch always seems to return any. Is that expected or am I doing something wrong?

tyxla commented 1 year ago

I'm curious: in my testing, while passing the store to useSelect does correctly provide types, passing the store to useDispatch always seems to return any. Is that expected or am I doing something wrong?

This is expected, @sirbrillig, because useDispatch types were added later in https://github.com/WordPress/gutenberg/pull/43528.

We'll improve the situation with useDispatch types as soon as we upgrade to a newer version of the @wordpress packages, which is on our radar.

sirbrillig commented 1 year ago

Sounds good! I just wanted to make sure I was doing it right!

tyxla commented 1 year ago

Many of the usages of those stores are in Gutenboarding, which I found to be unused. See #74475 where I explain in detail and suggest its removal.

Landing that will make part of the work refactoring those stores easier, since some may end up unused.

noahtallen commented 1 year ago

I ran into an issue with block-editor-nux (#74454). Essentially, with the way the wp data persistence plugin works, it only works if you use registerStore and not the other methods. However, there has been a lot of work and deprecation around the plugin layer in general, so there's not a straightforward solution. Hopefully we can have a conversation about that in core here: https://github.com/WordPress/gutenberg/issues/11449

noahtallen commented 1 year ago

In this PR, there are suggestions that using the old registration approach for data stores improves tree shaking. Thoughts?

tyxla commented 1 year ago

In this PR, there are suggestions that using the old registration approach for data stores improves tree shaking. Thoughts?

I'm not concerned about this for a few reasons:

Calypso is far from being 100% ESM, and that prevents webpack from properly being able to identify modules as side-effect-free. That means that for any module that we don't specifically mark as side-effect-free (through "sideEffects": false in a corresponding package.json file), tree shaking won't really work. We don't follow that practice often in Calypso, therefore we don't leverage tree shaking properly either. With that in mind, what difference does this make? Nothing measurable from what I can expect.
The additional register() function is an additional layer of indirection, yet another abstraction, which I'm not sure is justified.
With the register() function we had to check the store for existence before registration. That pattern to me was yet another code smell and indication that we're likely not doing something properly, especially given that this pattern doesn't exist in Gutenberg.
The additional register() function felt like a naming conflict. Is this our register() data store registration function? Is it @wordpress/data's register() function? One would have to look at the imports to be sure.
The new approach follows the core Gutenberg patterns for store registration. If we discover a better approach for store registration, I'd expect that would come from there first, and then would be adopted from Calypso, not the other way around. Ideally any research, discovery and new feature work in data and data store registration APIs should be done there, and then we should follow suite in Calypso.
The new approach is simpler and more straightforward and allows type inference without the additional magic that was required before. Given that it's the default way of registering stores in Gutenberg, I only expect type support will even improve over time, like it historically did with adding types for useDispatch() a few months ago.

noahtallen commented 1 year ago

Nice explanation, thanks! One related thought I had is that importing anything from @automattic/data-stores probably registers every single store in the package, given that the file import in the top-level index file of that package will cause every store to register. This didn't happen before. I don't immediately have any concerns about it, though.

tyxla commented 1 year ago

Nice explanation, thanks! One related thought I had is that importing anything from @automattic/data-stores probably registers every single store in the package, given that the file import in the top-level index file of that package will cause every store to register. This didn't happen before. I don't immediately have any concerns about it, though.

That's a valid one, indeed. I don't very much like the idea of re-exporting everything through the main package anyway, especially given that we have side effects. One way to beat that would be to not use a primary index and re-export there, but rather to import from the specific stores only, using bare module specifier resolution:


import { AutomatedTransferEligibility } from '@automattic/data-stores/automated-transfer-eligibility';

jsnajdr commented 1 year ago

importing anything from @automattic/data-stores probably registers every single store in the package

The every-store-registration shouldn't be happening. The data-stores package has the sideEffects: false flag enabled, and when there are re-exports like this in data-stores/index.js:

import * as Foo from './foo';
import * as Bar from './bar';

export { Foo, Bar };

Then webpack will load ./foo only if its exports are actually imported anywhere. Reexports are "pass-through", i.e., if some useFoo.js file imports Foo:

import { Foo } from 'data-stores';

webpack will not consider the intermediate index.js file, and will treat it as "useFoo.js is importing Foo from data-stores/foo.js". Cut out the middleman!

The caveat is that the Foo store will be registered only if Foo is imported anywhere. In other words, you are kind of forced to use the new store description convention:

import { Foo } from 'data-stores';

registry.select( Foo.store ).getFoo();

noahtallen commented 1 year ago

In other words, you are kind of forced to use the new store description convention

Nice, thanks for the explanation. This side effect stuff can get tricky :)

We are migrating how these stores are used at the same time. Previously, you still needed to import Foo store and register it somewhere in the app. In most cases, the consumer also re-exported the store key and then used that to access the stores. So it's not a massive change -- just removing one or two layers of indirection

Automattic / wp-calypso