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

Core Data and typing the un-typable #34190

Open dmsnell opened 3 years ago

dmsnell commented 3 years ago

Recently I was working with @sarayourfriend (#33139) and @sirreal to try and build types for data and ran into a roadblock I couldn't get past. That module, and I think by extension most of Gutenberg, is untypable in TypeScript the way we would want and this is all because of its dynamic ability to change at runtime. There are no static types because the editor is the product of its own code plus whatever other scripts run when booting up and attaching hooks or filters.

When thinking about typing something like data we have to admit a kind of lie to TypeScript, a cast to tell it "this is what I really have even though you can't see it." I'm creating this issue to highlight this obstacle and talk about how to handle it in a way that helps developers using data and any other modules we type.

What's the problem?

data provides redux stores that components and plugins can interact with. There are some core stores provided by Gutenberg, for example providing getCurrentUser and saveEntityRecord from core-data, and there are custom stores provided by plugin code.

When we write types for the module I believe we should communicate that those pieces provided by core are there. At the same time I want to be able to allow a developer to use @wordpress/data and supply their own type extensions and have their custom stores in the types.

The problem is that if I want to allow this, as a type designer, I'm almost forced to have someone create their own god-type and pass it around to every call of every exported method from core data. These registries and stores and extensions are stringly-typed too, making it even more difficult for me to reason about. That is, I know that we might hit an inevitable type block when passing unknown objects to the useSelect and useDispatch functions (and friends), but for some cases we should be able to recall the right properties by passing the store name or the store object.

What's even worse is that plugins can muck around with the core-provided functionality so even if we only export the core-provided stuff we will still be wrong

useSelect('my-store').
                     ^ this should auto-complete, suggesting, `getSettings`, for example

I'm fine assuming someone will have to define their custom store. Let's give this in the problem statement:

import type { CoreStores } from '@wordpress/data'

type MyStore = CoreStores | CatStore | DogStore;

So we can even assume someone will need to use useSelect<MyStore>(…) and be okay with that. It's not the most ideal, because we have to pass that type parameter everywhere, but it's reasonable.

What I've tried

I've tried a few formulations of this in the TypeScript playground and have had trouble getting the types right. This may be my own deficiency, but it comes to the point where I either have to concede to adding manual type parameters (used as casts) everywhere, or to give up auto-complete and concede to writing ?. everywhere.

by using a "global type" and declaring that I'm able to get everything I want, but at a heavy cost: it's an absolutely terrible thing to do and I think requires breaking the unaugmented core types.

Even with these concessions I can't seem to get things consistent. I would expect that if I provide (necessary) casts like useSelect(select => select<MyStore>('my/cats').whiskerCount()) then it should all work, but this breaks in ways I don't understand.

what I really really want

I want to have some kind of "module functor import" but I don't know what that means exactly or what I need to say to say what I want. if I imagine some syntax it would look like this…

import type { MyStore } from 'src/extensions/store';
import <MyStore>{ useDispatch, useSelect } from '@wordpress/data';

this would type the functions with that custom parameter. this isn't possible unfortunately. a few people have similar wants.

there's a strange dance of functions produced by these methods and it's almost like we have to pass the type into the module so that it can pass its generated functions out with the same type, hence the module functor idea (like with OCaml). I sure wish I understood this stuff better so I could talk about it more eloquently.

what I think we should be able to get away with

Given that we can't have the imports automatically apply some custom type to the whole module, and given that we can't supply any kind of placeholder type, I think it should be reasonable to do one of two things:

Pass the type on every call and have the types thread through the callbacks and chains

// this should autocomplete and type according to our expectations
const catSelect = useSelect<MyStore>(select => select('my/cats').whiskerCount());

Create a wrapper function to adopt the type and use it instead

import { useSelect as rawUseSelect } from '@wordpress/data';

const useSelect = rawUseSelect<MyStore>;

const catSelect = useSelect(select => select('my/cats').whiskerCount());

To date I have had incredible trouble getting these to work without hassle. Can you help? What are your thoughts? Have you worked on this problem too? What do you think should be the goal for typing packages like @wordpress/data?

sirreal commented 3 years ago

CC: @dsifford

dsifford commented 3 years ago

Assuming I'm understanding the issue you're speaking of here correctly @dmsnell...

I got around this issue by using typescript module augmentation in a project that I worked on at the time:

https://github.com/dsifford/academic-bloggers-toolkit/blob/master/types/augments.d.ts

A similar strategy was done in several of the DefinitelyTyped repos:

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/4e1a55e0b0736de505e4a1f6f3539680359ff133/types/wordpress__editor/index.d.ts

dmsnell commented 3 years ago

@dsifford is this essentially duplicating the core types in our own libraries in order to provide the overrides?

dsifford commented 3 years ago

@dsifford is this essentially duplicating the core types in our own libraries in order to provide the overrides?

Not duplicating. Augmenting the existing types to shim in types added by our own libraries. All the existing types work as expected, but, for example, when I type select("abt/data") or dispatch("abt/data"), I get the completion for my libraries functionality as well.

This is fairly standard and I wasn't turned off by having to add that into my project.

sarayourfriend commented 3 years ago

FWIW I believe Calypso does the same thing

https://github.com/Automattic/wp-calypso/blob/trunk/packages/data-stores/src/i18n/index.ts#L29:L32

If this is an established and widely used pattern maybe it's worth investing more into it? It seems to work well even if it's not as nice as something that can infer everything from one main store type.

dmsnell commented 3 years ago

Not duplicating. Augmenting the existing types

I'm thinking more about how changes to the upstream types cascade down and whether they might break our user-defined augmentations, or be hidden by the same

also, does that handle the cases like useSelect(select => select('my/store').property())?

dsifford commented 3 years ago

also, does that handle the cases like useSelect(select => select('my/store').property())?

Fairly certain that it should.

dmsnell commented 3 years ago

When I add this augmentation it seems like I have to augment every function I use, which is one of the reasons I was not a fan of this kind of approach. Am I doing something wrong?

declare module '@wordpress/data' {
  function useSelect(key: Key): MyStoreStuff
}
import { useDispatch } from '@wordpress/data';
         ^^^ TS2305: Module '"@wordpress/data"' has no exported member 'useDispatch'.

there are currently eight exports in the @wordpress/data file. it seems like it would be a big ask to have every develop re-type every export from every WP package they depend on.

sarayourfriend commented 3 years ago

@dmsnell I think you only have to overload the dispatch and select functions as those are the cores of everything else, right?

dsifford commented 3 years ago

@dmsnell I think you only have to overload the dispatch and select functions as those are the cores of everything else, right?

Precisely

dmsnell commented 3 years ago

I think you only have to overload the dispatch and select functions as those are the cores of everything else, right?

this feels like a pragmatic approach at a project-level, but I think we'll have the same issue in a number of other packages and I feel like we shouldn't require people to do all that work just to use the core packages.

I've worked in a number of similar situations where the types flowed more gracefully and provided all the benefits but didn't require all the manual (and bug-prone) work.

matt-sanders commented 1 year ago

This is a bit of an older thread, but is there currently any decent way around this? I'm trying to do the following, but keep receiving type errors:

const editorContent = useSelect( select => select('core/editor').getEditedPostContent() )

Currently the @wordpress/editor package is untyped, which means that the following also doesn't work:

import { store as editorStore } from '@wordpress/editor'
const editorContent = useSelect( select => select(editorStore).getEditedPostContent() )

I tried declaring the editor module manually and I got very close, but it still didn't work:

declare module '@wordpress/editor' {
  import {
    StoreDescriptor,
    ReduxStoreConfig,
  } from '@wordpress/data/src/types'

  type Selectors = {
    getEditedPostContent: () => string
  }
  type StoreConfig = ReduxStoreConfig<unknown, Record<string, any>, Selectors>
  type Store = StoreDescriptor<StoreConfig>

  export const store: Store
}

This eventually typed select(editorStore) as { getEditedPostContent: unknown } for some reason.

Any help or current workarounds is much appreciated. Currently we just typecast:

(select('core/editor') as WPDataCoreEditor).getEditedPostContent()

Which is less than ideal. Especially when using it in many places.

dmsnell commented 1 year ago

I'm not sure what to recommend other than what you've already done there @matt-sanders. Someone else might have more helpful advice. This might be resolved one day in the library itself, but for now, typecasting may be the most appropriate practical solution.

matt-sanders commented 1 year ago

Ok no worries, thanks @dmsnell !