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

2691 Flow libraries with tests, converted from Typescript #590

Closed
wants to merge 8 commits into from

Conversation

joarwilk
Copy link

@joarwilk joarwilk commented Jan 3, 2017

I made a typescript -> flow converter, and I'd like to share the state of that converter through this PR.

Obviously, this PR is not meant to be merged. It's 1.8 million lines of auto-generated code which no maintainer would ever be able to review. However, I'd like to spawn some discussion regarding this set of library files.

Currently, some library definitions work really well out of the box, some work with some manual adjustments and some will probably never work. I'm wondering where this set should live. Should it have its own repo of non-quality assured definitions? Should it be a base that flow-typed maintainers can use to quickly get started with new definition implementations? Should it never have been created in the first place?

I'm very curious to hear what you guys feel about this, and how we move forward.

I'm also in need of help from someone with intricate knowledge of flow, as there's a couple of cases where I feel I'm a bit stuck. Please send me a tweet if you feel you can help out.

@turadg
Copy link
Contributor

turadg commented Jan 4, 2017

I'm wondering where this set should live. Should it never have been created in the first place?

Pshaw! Your converter is awesome. :-) If you mean making a giant PR, yeah I think it would be easier to review as a repo where one can more easily pull up each file of interest.

Should it have its own repo of non-quality assured definitions? Should it be a base that flow-typed maintainers can use to quickly get started with new definition implementations?

I'm not a maintainer, but my understanding is that flow-typed is meant to have high quality definitions that people can rely on. The flow-typed install command doesn't have a way to filter out potentially problematic defs.

That might be a good feature. In the meantime, there's another defs repo that is "more quick & dirty" for not-ready-for-primetime defs: https://github.com/marudor/flowInterfaces/

@ryyppy
Copy link
Contributor

ryyppy commented Jan 4, 2017

Hey! Awesome work 💪.

I looked through a few definitions and there is one major issue:

The definitions need to be wrapped in a declare module 'x' scope to prevent global definitions..

For global declarations, we also need a $npm$[modulename] namespace prefix...

other than that... I think we should really try a marathon and slowly create PRs one by one ... we are happy to put the generator instructions in our Contribution guidelines, if the generator works with our conventions :-)

If people are really urging to use d.ts files, we can refer them to generate them for themselves, since they have to put their libdef files in git anyways... it's a one time work for their specific dependencies

@rattrayalex
Copy link

I think we should really try a marathon and slowly create PRs one by one

Speaking as a user, I would love (love love love) that! Having the entire contents of definitely-typed hand-vetted for flow would make adopting flow more widely a much easier decision.

One big question would be how to keep things updated; should a bot be built to monitor changes in definitely-typed and submit generated PR's to the corresponding flow files?

Two sidenotes:

  1. I imagine the definitely-typed folks would appreciate any upstream changes you could suggest; having the two teams work well together would be a boon to both communities I'm sure.
  2. One benefit of the marathon you suggest is that it may be much easier to spot any small, systematic inaccuracies in the generator, and make those changes to the generator, rather than just to the generated code itself.

@joarwilk
Copy link
Author

joarwilk commented Jan 8, 2017

I'm happy guys like it! I would strongly advise against doing any manual work on this set right now, as (large) parts of its output probably will change in the near future. Ideally some sort of automated process would merge changes upstream, like @rattrayalex was talking about.

The definitions need to be wrapped in a declare module 'x' scope to prevent global definitions..

Now this is a tricky part. Namespaces in TS have no counterpart what so ever in Flow, and so the solution I came up with was to break out each prop of a namespace into the global scope with a prefix. It works, but as you noted it pollutes the global scope. I'd LOVE for some help in this regard as I cant come up with any other solution.

One thing I tried was converting each namespace into a module declaration and then importing any used props from that module. Flow doesnt seem to support importing props from a module declared in the same file, so that didnt work.

One solution would be to keep track of the types of the namespace, and then insert definitions anywhere they are referenced. This will lead to HUGE library files however (i.e. lodash will be 50x larger).

@jamiebuilds
Copy link
Contributor

jamiebuilds commented Jan 9, 2017

I talked to @jeffmo at some point about having another directory in flow-typed titled "in-progress" or something. It might make sense to put these in that.

It might be nice to update the CLI to install these but warn the user that they are not vetted for quality and may need improvement.

Also, how are the versions decided here?

@jamiebuilds
Copy link
Contributor

Rebased this

@jamiebuilds
Copy link
Contributor

Idea: Maybe we could include this converter and definitely-typed in the CLI and use it as a fallback warning the user when we do. That way it would stay in sync with DT until someone has manually vetted it for quality.

@joarwilk
Copy link
Author

joarwilk commented Jan 10, 2017

Also, how are the versions decided here?

DT doesn't do versions so I defaulted to 1.0 (placeholder). If we want to automate the versions we could correlate the latest stable version of the module at the time of the DT commit, which should be good enough in most cases. Also there's usually some information in the first comment of the definition file as well, maybe extract that?

Idea: Maybe we could include this converter and definitely-typed in the CLI and use it as a fallback warning the user when we do. That way it would stay in sync with DT until someone has manually vetted it for quality.

I like this idea, but it may require some sort of rollback process. What if an unvetted lib file causes a false error? Library files unfortunately feels like an "100% or nothing" kind of deal.

Fortunately there's a lot of tests(~40% of defs?) compiled from DT. If I can make them all pass the overall quality should be good enough for an "in-progress" merge.

@jamiebuilds
Copy link
Contributor

One broken conversion piece:

`declare export interface` is not supported. Use `export interface` instead.

@joarwilk
Copy link
Author

joarwilk commented Mar 9, 2017

Closing this for now until the next major improvement in the converter (slowly getting there). Unnecessary burden for the maintainers to have a stale open PR.

@sibelius
Copy link
Contributor

this look like a great idea, any progress on this?

@gantoine
Copy link
Member

We'd need a group of champions to slowly start translating these definitions, and a need from the community for the translated libdefs. Are you looking to volunteer? 😛

In all seriousness, prioritizing libdefs to be translated based on open issues and library popularity would be a good place to start.

@graingert
Copy link

@joarwilk any status on that? There's a few open PRs...

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.

8 participants