Skip to content

Commit

Permalink
Omit built-in context prop if user component props include context
Browse files Browse the repository at this point in the history
Connected components allow users to pass in a `ReactReduxContext`
instance as a prop named `context`. We also do internal checks to
see if that really _is_ a context instance, in case the user passed
a real value as `props.context`.

However, the TS types did not reflect this - `props.context` was always
a `ReactReduxContext` type, and if users did have a component that
accepted a prop named `context`, this would cause a type clash.

In this case, the types didn't reflect the reality of runtime behavior.

By extracting "the props of the component" and omitting the built-in
`context` field type if appropriate, this now works.
  • Loading branch information
markerikson committed Sep 23, 2022
1 parent bbc546e commit f40d82d
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 14 deletions.
16 changes: 8 additions & 8 deletions src/components/connect.tsx
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
/* eslint-disable valid-jsdoc, @typescript-eslint/no-unused-vars */
import hoistStatics from 'hoist-non-react-statics'
import React, { useContext, useMemo, useRef } from 'react'
import React, { ComponentType, useContext, useMemo, useRef } from 'react'
import { isValidElementType, isContextConsumer } from 'react-is'

import type { Store } from 'redux'

import type {
AdvancedComponentDecorator,
ConnectedComponent,
InferableComponentEnhancer,
InferableComponentEnhancerWithProps,
ResolveThunks,
DispatchProp,
ConnectPropsMaybeWithoutContext,
} from '../types'

import defaultSelectorFactory, {
Expand Down Expand Up @@ -465,18 +465,18 @@ function connect<

const Context = context

type WrappedComponentProps = TOwnProps & ConnectProps

const initMapStateToProps = mapStateToPropsFactory(mapStateToProps)
const initMapDispatchToProps = mapDispatchToPropsFactory(mapDispatchToProps)
const initMergeProps = mergePropsFactory(mergeProps)

const shouldHandleStateChanges = Boolean(mapStateToProps)

const wrapWithConnect: AdvancedComponentDecorator<
TOwnProps,
WrappedComponentProps
> = (WrappedComponent) => {
const wrapWithConnect = <TProps,>(
WrappedComponent: ComponentType<TProps>
) => {
type WrappedComponentProps = TProps &
ConnectPropsMaybeWithoutContext<TProps>

if (
process.env.NODE_ENV !== 'production' &&
!isValidElementType(WrappedComponent)
Expand Down
15 changes: 9 additions & 6 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,6 @@ export interface DispatchProp<A extends Action = AnyAction> {
dispatch: Dispatch<A>
}

export type AdvancedComponentDecorator<TProps, TOwnProps> = (
component: ComponentType<TProps>
) => ComponentType<TOwnProps>

/**
* A property P will be present if:
* - it is present in DecorationTargetProps
Expand Down Expand Up @@ -92,10 +88,17 @@ export type ConnectedComponent<
WrappedComponent: C
}

export type ConnectPropsMaybeWithoutContext<TActualOwnProps> =
TActualOwnProps extends { context: any }
? Omit<ConnectProps, 'context'>
: ConnectProps

// Injects props and removes them from the prop requirements.
// Will not pass through the injected props if they are passed in during
// render. Also adds new prop requirements from TNeedsProps.
// Uses distributive omit to preserve discriminated unions part of original prop type
// Uses distributive omit to preserve discriminated unions part of original prop type.
// Note> Most of the time TNeedsProps is empty, because the overloads in `Connect`
// just pass in `{}`. The real props we need come from the component.
export type InferableComponentEnhancerWithProps<TInjectedProps, TNeedsProps> = <
C extends ComponentType<Matching<TInjectedProps, GetProps<C>>>
>(
Expand All @@ -107,7 +110,7 @@ export type InferableComponentEnhancerWithProps<TInjectedProps, TNeedsProps> = <
keyof Shared<TInjectedProps, GetLibraryManagedProps<C>>
> &
TNeedsProps &
ConnectProps
ConnectPropsMaybeWithoutContext<TNeedsProps & GetProps<C>>
>

// Injects props and removes them from the prop requirements.
Expand Down
37 changes: 37 additions & 0 deletions test/typetests/connect-options-and-issues.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
createStoreHook,
TypedUseSelectorHook,
} from '../../src/index'
import { ConnectPropsMaybeWithoutContext } from '../../src/types'

import { expectType } from '../typeTestHelpers'

Expand Down Expand Up @@ -881,3 +882,39 @@ function testPreserveDiscriminatedUnions() {
;<ConnectedMyText type="localized" color="red" />
;<ConnectedMyText type="localized" color="red" params={someParams} />
}

function issue1187ConnectAcceptsPropNamedContext() {
const mapStateToProps = (state: { name: string }) => {
return {
name: state.name,
}
}

const connector = connect(mapStateToProps)

type PropsFromRedux = ConnectedProps<typeof connector>

interface IButtonOwnProps {
label: string
context: 'LIST' | 'CARD'
}
type IButtonProps = IButtonOwnProps & PropsFromRedux

function Button(props: IButtonProps) {
const { name, label, context } = props
return (
<button>
{name} - {label} - {context}
</button>
)
}

const ConnectedButton = connector(Button)

// Since `IButtonOwnProps` includes a field named `context`, the final
// connected component _should_ use exactly that type, and omit the
// built-in `context: ReactReduxContext` field definition.
// If the types are broken, then `context` will have an error like:
// Type '"LIST"' is not assignable to type '("LIST" | "CARD") & (Context<ReactReduxContextValue<any, AnyAction>> | undefined)'
return <ConnectedButton label="a" context="LIST" />
}

0 comments on commit f40d82d

Please sign in to comment.