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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: basic support for inferrable types #1407

Merged
merged 1 commit into from Dec 3, 2022
Merged

feat: basic support for inferrable types #1407

merged 1 commit into from Dec 3, 2022

Conversation

swnf
Copy link
Contributor

@swnf swnf commented Sep 15, 2022 •
edited by github-actions bot

This PR adds support to infer the type of properties without an explicit type. For example:

export class Foo {
  a = false;
}

Previously, properties without an explicit type were always ignored. With this PR they will get their correct type from the type checker.

This PR fixes #1406.

Version

Published prerelease version: v1.2.0-next.3

Changelog

馃帀 This release contains work from new contributors! 馃帀

Thanks for all your work!

鉂わ笍 null@swnf

鉂わ笍 Thomas (@thomaswr)

鉂わ笍 Arthur Fiorette (@arthurfiorette)

鉂わ笍 Sean Keenan (@sean9keenan)

馃殌 Enhancement

馃悰 Bug Fix

馃敥 Dependency Updates

Authors: 7

@swnf swnf mentioned this pull request Sep 15, 2022
Closed
arthurfiorette
arthurfiorette suggested changes Sep 16, 2022
View reviewed changes
Copy link
Contributor

@arthurfiorette arthurfiorette left a comment

Choose a reason for hiding this comment

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

Hey @swnf, I'm not dedicated to this project or either a main contributor. But I arrieved at your PR randomly and decided to stick my nose for a bit, you can only use what you find useful :)

test/valid-data/class-single/main.ts Outdated Show resolved Hide resolved
src/Utils/inferrableType.ts Outdated Show resolved Hide resolved
src/Utils/inferrableType.ts Outdated Show resolved Hide resolved
src/NodeParser/InterfaceAndClassNodeParser.ts Outdated Show resolved Hide resolved
@swnf
Copy link
Contributor Author

swnf commented Sep 17, 2022 •
edited

Thanks for your feedback @arthurfiorette. I made some changes based on it. However, I'm not so sure anymore if my approach makes sense. There does not seem to be any advantage in parsing the AST of the initializer (except maybe for an AsExpression). It might make more sense to just use the type checker directly to get the type of a member if it has no explicit type (would probably depend on #1386).

@ThatOneAwkwardGuy ThatOneAwkwardGuy mentioned this pull request Sep 26, 2022
Closed
@domoritz
Copy link
Member

Thanks for the pull request. I think using the type checker directly would make sense if it makes the code a lot simpler. We mainly use the AST to preserve aliases but that isn't an issue here with inferred types.

@domoritz
Copy link
Member

@swnf would you be able to revise this pull request?

@swnf
Copy link
Contributor Author

swnf commented Oct 16, 2022

Yes, I think I can update it next week.

@swnf
Copy link
Contributor Author

swnf commented Oct 17, 2022

I've updated my code. It now uses the type checker and works for all types. I will rebase the PR once #1386 is merged.

@domoritz
Copy link
Member

I merged #1386

@swnf
Copy link
Contributor Author

swnf commented Oct 17, 2022

I've rebased the PR. I think it is now ready for review.

@swnf
Copy link
Contributor Author

swnf commented Nov 25, 2022

@domoritz can you have a look at this PR? It would help me with some eslint issues.

@domoritz domoritz merged commit 62a17a9 into vega:next Dec 3, 2022
@github-actions github-actions bot mentioned this pull request Dec 9, 2022
Merged
This was referenced Dec 21, 2022
Merged
Merged
@github-actions
Copy link

馃殌 PR was released in v1.2.0 馃殌

@swnf swnf deleted the inferrable-types-support branch December 24, 2022 13:24
@Zamiell Zamiell mentioned this pull request Jan 13, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arthurfiorette arthurfiorette arthurfiorette requested changes

Assignees
No one assigned
Labels
prerelease
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support inferrable types
3 participants
@swnf @domoritz @arthurfiorette