Skip to content
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.

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

Omit built-in context prop if user component props include context #1958

Merged
merged 2 commits into from
Sep 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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,>(
Copy link
Member

Choose a reason for hiding this comment

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

is the comma a typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it was actually necessary!

When I removed it, TS thought that <TProps> was the start of a JSX element, because this is a .tsx file. I had to leave it in to convince TS "nope, this is a generic type declaration" (presumably similar to how in Python (x) is just parentheses, and (x,) is a tuple).

WrappedComponent: ComponentType<TProps>
) => {
type WrappedComponentProps = TProps &
ConnectPropsMaybeWithoutContext<TProps>

if (
process.env.NODE_ENV !== 'production' &&
!isValidElementType(WrappedComponent)
Expand Down
30 changes: 19 additions & 11 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,22 +88,34 @@ export type ConnectedComponent<
WrappedComponent: C
}

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

type Identity<T> = T
export type Mapped<T> = Identity<{ [k in keyof T]: T[k] }>

// 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>>>
>(
component: C
) => ConnectedComponent<
C,
DistributiveOmit<
GetLibraryManagedProps<C>,
keyof Shared<TInjectedProps, GetLibraryManagedProps<C>>
> &
TNeedsProps &
ConnectProps
Mapped<
DistributiveOmit<
GetLibraryManagedProps<C>,
keyof Shared<TInjectedProps, GetLibraryManagedProps<C>>
> &
TNeedsProps &
ConnectPropsMaybeWithoutContext<TNeedsProps & GetProps<C>>
>
>

// Injects props and removes them from the prop requirements.
Expand Down
39 changes: 38 additions & 1 deletion 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 @@ -464,7 +465,7 @@ function TestOptionalPropsMergedCorrectly() {
}
}

connect(mapStateToProps, mapDispatchToProps)(Component)
const Connected = connect(mapStateToProps, mapDispatchToProps)(Component)
}

function TestMoreGeneralDecorationProps() {
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" />
}