Skip to content

Convert Context/Provider/Subscription to Typescript #1742

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Jun 26, 2021
Merged

Convert Context/Provider/Subscription to Typescript #1742

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
211cdff
Context.ts
Vannevelj Jun 26, 2021
5e6c80d
allow JSX in tsconfig
Vannevelj Jun 26, 2021
286a131
Provider.tsx
Vannevelj Jun 26, 2021
5d71c44
Subscription.tsx
Vannevelj Jun 26, 2021
1e49d3c
zero-out listeners
Vannevelj Jun 26, 2021
7865ed2
rollup supports .tsx?
Vannevelj Jun 26, 2021
2966e30
Fixed compilation error
Vannevelj Jun 26, 2021
088504b
removed semi-colons to stay in line with existing styles
Vannevelj Jun 26, 2021
420891d
Use types that came from the typedef package
Vannevelj Jun 26, 2021
e4eacc9
linting
Vannevelj Jun 26, 2021
6a617e2
less changes
Vannevelj Jun 26, 2021
f1a4dbc
Removed PropTypes
Vannevelj Jun 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rollup.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import pkg from './package.json'

const env = process.env.NODE_ENV

const extensions = ['.js', '.ts', '.json']
const extensions = ['.js', '.ts', '.tsx', '.json']
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


const config = {
input: 'src/index.js',
Expand Down
7 changes: 6 additions & 1 deletion src/components/Context.js → src/components/Context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import React from 'react'
import type Subscription from '../utils/Subscription'

export const ReactReduxContext = /*#__PURE__*/ React.createContext(null)
export type ReduxContextProps = {
store: any
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@markerikson markerikson Jun 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

subscription: Subscription
}
export const ReactReduxContext = /*#__PURE__*/ React.createContext<ReduxContextProps | null>(null)

if (process.env.NODE_ENV !== 'production') {
ReactReduxContext.displayName = 'ReactRedux'
Expand Down
8 changes: 4 additions & 4 deletions src/components/Provider.js → src/components/Provider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import React, { useMemo } from 'react'
import React, { ReactNode, useMemo } from 'react'
import PropTypes from 'prop-types'
import { ReactReduxContext } from './Context'
import { ReactReduxContext, ReduxContextProps } from './Context'
import Subscription from '../utils/Subscription'
import { useIsomorphicLayoutEffect } from '../utils/useIsomorphicLayoutEffect'

function Provider({ store, context, children }) {
function Provider({ store, context, children }: { store: any, context?: React.Context<ReduxContextProps>, children: ReactNode }) {
const contextValue = useMemo(() => {
const subscription = new Subscription(store)
subscription.onStateChange = subscription.notifyNestedSubs
Expand All @@ -25,7 +25,7 @@ function Provider({ store, context, children }) {
}
return () => {
subscription.tryUnsubscribe()
subscription.onStateChange = null
subscription.onStateChange = undefined
}
}, [contextValue, previousState])

Expand Down
44 changes: 27 additions & 17 deletions src/utils/Subscription.js → src/utils/Subscription.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,16 @@ import { getBatch } from './batch'
// well as nesting subscriptions of descendant components, so that we can ensure the
// ancestor components re-render before descendants

const nullListeners = { notify() {} }
type Listener = {
callback: () => void
next: Listener | null
prev: Listener | null
}

function createListenerCollection() {
const batch = getBatch()
let first = null
let last = null
let first: Listener | null = null
let last: Listener | null = null

return {
clear() {
Expand Down Expand Up @@ -37,10 +41,10 @@ function createListenerCollection() {
return listeners
},

subscribe(callback) {
subscribe(callback: () => void) {
let isSubscribed = true

let listener = (last = {
let listener: Listener = (last = {
callback,
next: null,
prev: last,
Expand Down Expand Up @@ -71,29 +75,35 @@ function createListenerCollection() {
}
}

type ListenerCollection = ReturnType<typeof createListenerCollection>

export default class Subscription {
constructor(store, parentSub) {
private store: any
private parentSub?: Subscription
private unsubscribe?: () => void
private listeners?: ListenerCollection
public onStateChange?: () => void

constructor(store: any, parentSub?: Subscription) {
this.store = store
this.parentSub = parentSub
this.unsubscribe = null
this.listeners = nullListeners
this.unsubscribe = undefined
this.listeners = undefined

this.handleChangeWrapper = this.handleChangeWrapper.bind(this)
}

addNestedSub(listener) {
addNestedSub(listener: () => void) {
this.trySubscribe()
return this.listeners.subscribe(listener)
return this.listeners?.subscribe(listener)
}

notifyNestedSubs() {
this.listeners.notify()
this.listeners?.notify()
}

handleChangeWrapper() {
if (this.onStateChange) {
this.onStateChange()
}
this.onStateChange?.()
}

isSubscribed() {
Expand All @@ -113,9 +123,9 @@ export default class Subscription {
tryUnsubscribe() {
if (this.unsubscribe) {
this.unsubscribe()
this.unsubscribe = null
this.listeners.clear()
this.listeners = nullListeners
this.unsubscribe = undefined
this.listeners?.clear()
this.listeners = undefined
}
}
}
2 changes: 1 addition & 1 deletion tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
// "lib": [], /* Specify library files to be included in the compilation. */
"allowJs": true, /* Allow javascript files to be compiled. */
// "checkJs": true, /* Report errors in .js files. */
// "jsx": "preserve", /* Specify JSX code generation: 'preserve', 'react-native', 'react', 'react-jsx' or 'react-jsxdev'. */
"jsx": "react", /* Specify JSX code generation: 'preserve', 'react-native', 'react', 'react-jsx' or 'react-jsxdev'. */
"declaration": true, /* Generates corresponding '.d.ts' file. */
"emitDeclarationOnly": true,
// "declarationMap": true, /* Generates a sourcemap for each corresponding '.d.ts' file. */
Expand Down