Skip to content

fix #33427#33486

Merged
DanielRosenwasser merged 4 commits into
masterfrom
unknown repository
Oct 10, 2019
Merged

fix #33427#33486
DanielRosenwasser merged 4 commits into
masterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Sep 18, 2019

Fixes #33427

@msftclas
Copy link
Copy Markdown

msftclas commented Sep 18, 2019

CLA assistant check
All CLA requirements met.

@typescript-bot
Copy link
Copy Markdown
Collaborator

It looks like you've sent a pull request to update our 'lib' files. These files aren't meant to be edited by hand, as they consist of last-known good states of the compiler and are generated from 'src'. Unless this is necessary, consider closing the pull request and sending a separate PR to update 'src'.

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 19, 2019

@typescript-bot
I remove that commit with force-push!
@catb0t
Can you check if it's fixed as you intended??
@DanielRosenwasser
I wanna get some feedback!

@sploders101
Copy link
Copy Markdown

I don't speak for the typescript staff, but I still think the error is too vague.
I read "either string or number", and still think string | number, because that type can be either a string, or a number.
As suggested in #33427, I would like to see a suggestion as to what the dev is trying to do, like a Record.
I'm not sure how using a union as a key would result in a mapped type, but maybe somebody can clear that up, and we could potentially have that suggestion there too.

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 24, 2019

I think two messages mean:

either string | number -> you need to
use Record -> recommend you instead of union

How about thought of others?
Feel free to leave message :)

@DanielRosenwasser
Copy link
Copy Markdown
Member

Thoughts @fatcerberus?

@fatcerberus
Copy link
Copy Markdown

fatcerberus commented Oct 1, 2019

I agree with @sploders101 that the addition of "either" does next to nothing to improve clarity compared to the current message. My preference was always for the message that was suggested under Expected Behavior in #33427:

An index signature parameter type cannot be a union type. Consider using a mapped object type instead.

"Mapped object type" can be replaced with whatever is the name of the construct Record is defined in terms of. Alternatively, just suggest using Record since it's built in.

@ghost
Copy link
Copy Markdown
Author

ghost commented Oct 2, 2019

@fatcerberus
No worries :)
I didn't change only message.
I fixed the condition like this:

if (type.flags & TypeFlags.Union && allTypesAssignableToKind(type, TypeFlags.StringOrNumberLiteral, /*strict*/ true))

So we can see the error message what you want!
image

@DanielRosenwasser DanielRosenwasser merged commit 6162001 into microsoft:master Oct 10, 2019
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Union index signature parameter type has unclear / incorrect error

5 participants