Skip to content

feat(typing): use keyof inference to simplify definition #7797

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

Closed
wants to merge 1 commit into from

Conversation

HerringtonDarkholme
Copy link
Member

@HerringtonDarkholme HerringtonDarkholme commented Mar 12, 2018

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes (require new TS compiler)
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

Our typing is very complex. This pull request uses the new feature from TS 2.7 to combine record style and array style prop definition into one. microsoft/TypeScript#19227
A simpler type definition should be beneficial to both our users and we maintainers.

Kudos to @sandersn @DanielRosenwasser !

Note: this requires TS 2.7+. So it is expected to be included in a minor version update.

@DanielRosenwasser
Copy link

@yyx990803, I haven't tried this yet, but I think the TypeScript template in vue-cli should have this in the box if possible.

@DanielRosenwasser
Copy link

DanielRosenwasser commented Mar 12, 2018

Though, wait @HerringtonDarkholme, doesn't this mean that every type in props ends up being {}?

@HerringtonDarkholme
Copy link
Member Author

HerringtonDarkholme commented Mar 12, 2018

@DanielRosenwasser @sandersn has a following fix to inferany.
microsoft/TypeScript#21271

Copy link
Member

@ktsn ktsn left a comment

Choose a reason for hiding this comment

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

LGTM 😄

@yyx990803
Copy link
Member

There's a flow error causing the TS test to be skipped. Can we rebase against latest dev to get CI passing?

@yyx990803
Copy link
Member

@HerringtonDarkholme this PR seems to have been messed up by force pushes.

The type tests are failing after manual cherry-picking locally - can you take another look?

@HerringtonDarkholme
Copy link
Member Author

It looks like TypeScript has added a new inference priority for literal type as in https://github.com/Microsoft/TypeScript/pull/22525/files.

This means keyof T will not picked up as inference if mapped type exists. In our case, PropsDefinition = ArrayProp | RecordProp is such situation. To work around this, we will still need a function call overload as we are doing now....

But it does not simplify our type definition.

@HerringtonDarkholme
Copy link
Member Author

HerringtonDarkholme commented Dec 6, 2018

In short, we cannot achieve simpler definition in 2.6.

But we probably can rewrite the definition in Vue 3.0 with conditional types! I have used in my own project and it works quite well.

As for now, let's close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants