-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thasner
approved these changes
Jul 1, 2020
adidahiya
approved these changes
Jul 1, 2020
@timdorr Any chance you could take a look at this PR? 🙏 |
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. |
I'll merge this in, but I don't think we plan on releasing another 4.x version again. |
MirceaUngureanu
pushed a commit
to MirceaUngureanu/redux
that referenced
this pull request
Nov 22, 2020
👀 👀 👀 |
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
Former-commit-id: f361cc1
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
This was referenced May 24, 2024
This was referenced Jul 12, 2024
Open
This was referenced Jul 27, 2024
Open
This was referenced Sep 18, 2024
This was referenced Sep 19, 2024
This was referenced Sep 22, 2024
This was referenced Sep 24, 2024
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 tonever
inPreloadedState
. 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:The error on
initialState
in the last line: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] }