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

Migrate to TypeScript #115

Merged
merged 14 commits into from
Sep 13, 2020
Merged

Migrate to TypeScript #115

merged 14 commits into from
Sep 13, 2020

Conversation

zxbodya
Copy link
Contributor

@zxbodya zxbodya commented Jul 26, 2020

This might look a bit weird, considering this is one of the most important tools in the Flow community…

But, I believe, considering, a large part of flowgen is actually based on TypeScript - this makes a lot of sense.

Some background: did this mostly to make it easier to explore flowgen codebase for me, but I think this might be helpful to move for project in general.

Changes in PR:

  • updated dependencies
  • converted existing source code using https://github.com/zxbodya/flowts
  • did manual fixes, added some @ts-expect-errors with todo comments to make tsc happy (avoided doing much refactor - I think, this is better to be done separately, also I am not yet very familiar with TypeScript compiler API, to see how to better use it)
  • updated configuration for linter and babel to for with TypeScript

@orta
Copy link
Collaborator

orta commented Jul 26, 2020

I don't mind this personally, I feel like we'd need buy-in from @goodmind (who is probably the most active contributor and user for flowgen) for this to get merged though 👍

@joarwilk
Copy link
Owner

This PR made me chuckle. Looks good, but I dont contribute anymore so I agree with Orta, lets wait for more input.

@orta
Copy link
Collaborator

orta commented Aug 3, 2020

Alright, I'll give it one more week and I'll merge 👍

@meandmax
Copy link

meandmax commented Aug 3, 2020

@goodmind any thoughts?

@zxbodya I would like to add support to translate utility types like Pick and Omit to its target object type, since they are not available in flow. I would like to base this on your fork since it is much easier to reason about this in TS ... :) any thoughts or tips?

@zxbodya
Copy link
Contributor Author

zxbodya commented Aug 4, 2020

I would like to add support to translate utility types like Pick and Omit to its target object type, since they are not available in flow. I would like to base this on your fork since it is much easier to reason about this in TS ... :) any thoughts or tips?

have looked into exactly the same issue recently :)

in general case - it does not look possible to implement this utilities in Flow(at least I have not figured yet how to do it) - but it is possible to make something which, I would consider "good enough" - check helerTypes.ts here: zxbodya/flowts@cafce19#diff-754fea4ffe52a2096bdf2d7b852ad6de

also, there are special cases where it can be handled in better way, instead of having helper utility - see here: zxbodya/flowts@ee982fb

I did it outside of flowgen - in the experimental utility, which is based on babel ast transformation (just like flowts)… it is not yet finished: for practical use, currently - flowgen is better.

as an option, if you are looking for quick way to fix utility types in generated code - it should be easy to extract utility types related logic, from repo I linked, into separate babel codemod and just run it after flowgen on generated code

@meandmax
Copy link

meandmax commented Aug 4, 2020

Thank you so much @zxbodya that is really helpful indeed. I was wondering if it is possible to completely replace these utility types with the result of them like:

type A = {
  a: number,
  b: string,
  c: string 
};

type Picked = Pick<A, 'a', 'b'>;

to:

type Picked = {
  a: number,
  b: string,
};

What do you think about this idea? Btw. thank you for translating flowgen in TS 👍

@zxbodya
Copy link
Contributor Author

zxbodya commented Aug 4, 2020

Theoretically yes - I think, it should be possible to generate the result of applying Pick/Omit on the type.

But, I would suggest the approach should be different from what I did, and probably more complicated. Just babel ast - might not be enough to make it properly… Maybe using TypeScript compiler directly can help here (flowgen is probably better place to try doing it).

The only issue, I am thinking about - it might result in a lot of generated code, in some cases, which might be harder to read when using the definitions…

But generally, I think, this is a good idea - would be really interesting to see how it will work comparing to generic util types.

@orta
Copy link
Collaborator

orta commented Sep 11, 2020

OK, looks like goodmind has been pretty inactive all year - so let's not be blocked there.

@zxbodya - if you get this mergeable, I'll merge it 👍🏻

@zxbodya
Copy link
Contributor Author

zxbodya commented Sep 11, 2020

merged the changes from master, updated dependencies except typescript - which looks to require additional changes(so better to have it separate from this)

@orta
Copy link
Collaborator

orta commented Sep 13, 2020

Cool, works for me 👍🏻

@orta orta merged commit b206a05 into joarwilk:master Sep 13, 2020
@zxbodya zxbodya deleted the ts branch December 22, 2020 02:09
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.

4 participants