Convert Context/Provider/Subscription to Typescript

[Vannevelj]

Building on #1738, this converts Context, Provider and Subscription to Typescript.

Main things of note:

• undefined over null as it is more idiomatic in Typescript (able to use name?: Type as opposed to name: Type | null) -- only in two places though
• I've left store untyped as any for now since I've struggled with that type in the past, though happy to change it if you have a suggestion
• Typescript is flagging some unreachable code, more info down the PR

I'm getting an error "Cannot find module 'babel-plugin-polyfill-corejs2'" when running any npm command so I haven't been able to run tests locally -- if this issue rings a bell then let me know what I should do.

It's a small PR to start with as I wanted to make sure we're aligned on the approach that should be taken.

[netlify]

✔️ Deploy Preview for react-redux-docs ready!

🔨 Explore the source changes: f1a4dbc

🔍 Inspect the deploy log: https://app.netlify.com/sites/react-redux-docs/deploys/60d74d777426f4000b11eee2

😎 Browse the preview: https://deploy-preview-1742--react-redux-docs.netlify.app

[Vannevelj]

Because listener is initialized on line 47 with next: null, Typescript interprets it as the following structure:

However when it gets to this line, it causes an error for potentially accessing this null field. It doesn't seem to realise that listener.next would prevent that from getting accessed.

I'm not entirely sure what's going on here -- either Typescript is wrong that listener.next is always null (though not sure how that would be), or Typescript is wrong that this null check isn't sufficient. Is there another option?

[Vannevelj]

I sorted this by explicitly marking the variable as type Listener -- that ensured Typescript correctly interprets it as type Listener | null rather than null.

[Vannevelj]

I don't know rollup so I'm not sure if this is necessary, but it seemed reasonable

[markerikson]

Sweet, more community contributions! :) Lemme look this over

[markerikson]

A great start! I'd like to see the PR updated to copy-paste types defined in the React-Redux typedefs, but with any edits needed to be correct (ie, the context value has subscription but not storeState

[markerikson]

💬 Tell you what, let's use my favorite "stub this out" trick for converting stuff to TS.

Let's add a type $FixTypeLater = any somewhere, and use that here. That way we know this is something that needs to be addressed down the road.

Or, actually... the TS typedefs for React-Redux should already have the right-ish types that we need, and they specifically reference the core Redux Store type here:

https://www.unpkg.com/browse/@types/[email protected]/index.d.ts

export interface ReactReduxContextValue<SS = any, A extends Action = AnyAction> {
    store: Store<SS, A>;
    storeState: SS;
}

Hmm. Having said that, those types actually seem buggy, because we don't put the storeState in there, and haven't seen v6.

Still, we ought to try to reuse those typedefs as much as possible, because a lot of this stuff has been figured out for us already :) still a good idea to stub things out with $FixTypeLater in places, but we should be starting with these types as our baseline.

[Vannevelj]

I've added a new type FixTypeLater which is just an alias for any -- I'm not sure if I see all that much value in it (since you can just search for usages for any as well) but no strong feelings so don't mind adding it.

Good call on the @types -- I didn't think of using those. I've put them in and replaced the storeState field with subscription. Is this more like what you had in mind?

[markerikson]

Yep!

And yeah, the intent here is to distinguish between "an intentional use of any that needs to stay", vs "this is a placeholder use of any that needs to be fixed before we're done, we're just not messing with it right now to keep from having it block us"

[Vannevelj]

One thing I haven't done in this PR is remove the PropTypes:

react-redux/src/components/Provider.js

Lines 37 to 47 in a24b885

	if (process.env.NODE_ENV !== 'production') {
	Provider.propTypes = {
	store: PropTypes.shape({
	subscribe: PropTypes.func.isRequired,
	dispatch: PropTypes.func.isRequired,
	getState: PropTypes.func.isRequired,
	}),
	context: PropTypes.object,
	children: PropTypes.any,
	}
	}

I don't think it serves any purpose anymore so happy to drop it if you'd like me to.

[markerikson]

Yeah, I'm inclined to agree - I don't think they provide enough benefit at this point, and generally TS types for props have replaced the use of PropTypes from what I've seen. Go ahead and remove them.

[markerikson]

Looks great, merging!