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

Only apply mapped types to un-branded types #3805

Merged
merged 2 commits into from Jul 16, 2020

Conversation

invliD
Copy link
Contributor

@invliD invliD commented Jul 1, 2020

PR Type

Bugfix

Why should this PR be included?

#3485 fixed #3672 for TS 3.5, though starting with TS 3.6 that fix no longer worked. Branded types (e.g. string & { _brand: "a" }) now resolve to never in PreloadedState. This PR fixes #3672 for newer TS versions.

Note that this PR targets the 4.x branch, since we're using 4.0.5 today. If this PR is accepted I'll PR master with the same fix as well.

Bug Fixes

What is the current behavior, and the steps to reproduce the issue?

Branded strings cannot be passed to createStore as part of the preloaded state:

type BrandedString = string & { _brand: "a" };
interface IState {
    version: BrandedString;
}
const initialState: IState = {
    version: "1.0.0" as BrandedString,
};
createStore(…reducer…, initialState);

The error on initialState in the last line:

No overload matches this call.
  Overload 1 of 2, '(reducer: Reducer<IState, Action<any>>, enhancer?: StoreEnhancer<unknown, unknown> | undefined): Store<IState, Action<any>>', gave the following error.
    Argument of type 'IState' is not assignable to parameter of type 'StoreEnhancer<unknown, unknown>'.
      Type 'IState' provides no match for the signature '(next: StoreEnhancerStoreCreator<{}, {}>): StoreEnhancerStoreCreator<unknown, unknown>'.
  Overload 2 of 2, '(reducer: Reducer<IState, Action<any>>, preloadedState?: { version: never; } | undefined, enhancer?: StoreEnhancer<unknown, {}> | undefined): Store<...>', gave the following error.
    Argument of type 'IState' is not assignable to parameter of type '{ version: never; }'.
      Types of property 'version' are incompatible.
        Type 'BrandedString' is not assignable to type 'never'.ts(2769)

See playground, note that everything is fine for TS 3.5 but starts breaking at 3.6+.

What is the expected behavior?

The code compiles.

How does this PR fix the problem?

Similar to the proposal in microsoft/TypeScript#35992 this PR changes the mapped object type to only apply for types that are not primitives, since branded primitives are also objects, which is why the current code hits them. By reversing the condition we first exclude primitives and apply the mapped type to what's left: objects that aren't just a brand on a primitive.

Alternatively, the original suggestion in #3672 of allowing PreloadedState<S> | S should also work.

I could also change this PR to be slightly safer by making both the primitive escape hatch and the object matching explicit, e.g. { [K in keyof S]: S[K] extends string ? S[K] : S[K] extends object ? PreloadedState<S[K]> : S[K] }

@invliD
Copy link
Contributor Author

invliD commented Jul 14, 2020

@timdorr Any chance you could take a look at this PR? 🙏

@timdorr
Copy link
Member

timdorr commented Jul 14, 2020

To be honest, I haven't actually run into branded types before (at least not ones I wrote myself). This seems fine as-is, but can we do this for the master branch? I'd rather release type changes with the next major.

@timdorr
Copy link
Member

timdorr commented Jul 16, 2020

I'll merge this in, but I don't think we plan on releasing another 4.x version again.

@timdorr timdorr merged commit e23aa59 into reduxjs:4.x Jul 16, 2020
@invliD invliD deleted the sb/branded-types branch July 16, 2020 14:35
MirceaUngureanu pushed a commit to MirceaUngureanu/redux that referenced this pull request Nov 22, 2020
@markerikson
Copy link
Contributor

markerikson commented Apr 4, 2021

I don't think we plan on releasing another 4.x version again.

👀 👀 👀

webMasterMrBin pushed a commit to webMasterMrBin/redux that referenced this pull request Aug 21, 2021
webMasterMrBin pushed a commit to webMasterMrBin/redux that referenced this pull request Aug 21, 2021
webMasterMrBin pushed a commit to webMasterMrBin/redux that referenced this pull request Aug 21, 2021
webMasterMrBin pushed a commit to webMasterMrBin/redux that referenced this pull request Aug 21, 2021
webMasterMrBin pushed a commit to webMasterMrBin/redux that referenced this pull request Aug 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants