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

Adds flow definitions files #2719

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@IgorWilbert
Copy link
Contributor

IgorWilbert commented Feb 24, 2019

For #2647 . This a work in progress, any suggestion is welcome!

Change List

  • Adds flow definitions for function, classes, variables etc.

@IgorWilbert IgorWilbert force-pushed the IgorWilbert:adds-flow-definitions branch from ec45a63 to cb69c33 Feb 25, 2019

@IgorWilbert

This comment has been minimized.

Copy link
Contributor Author

IgorWilbert commented Feb 28, 2019

@ibgreen and @Pessimistress I am a bit confused about how we should do this. In #2645 and #2647 adding Flow type definitions is still a bit controversial. My solution was to treat our own code base as a third party and creating library definitions for the types and others, but I am not sure if the result is going to be good. Also, I just found an RFC that also proposes the implementation of a type system, but in a more "manual" way, without Flow or TypeScript. I still couldn't find a way how to automate libdefs writing, so the only advantage I see in creating Flow libdefs is for people who choose to use Flow in their projects, not in the development of Deck.gl itself. What do you think? Should I continue working on the libdefs approach or drop it and create a layer of types without third parties software?

@ibgreen

This comment has been minimized.

Copy link
Contributor

ibgreen commented Feb 28, 2019

adding Flow type definitions is still a bit controversial

@IgorWilbert Adding a separate type definitions file is not controversial. Changing the existing source code to use flow syntax is.

I just found an RFC that also proposes the implementation of a type system, but in a more "manual" way, without Flow or TypeScrip

We did already implement the runtime prop type system. Look in any layer source code, you will find prop type definitions, e.g. https://github.com/uber/deck.gl/blob/master/modules/layers/src/line-layer/line-layer.js#L31

I still couldn't find a way how to automate libdefs writing,

The only automation I could think of was a script that converts the above prop types to flow/typescript definition objects. I wrote a script to iterate over the props in all the layers here:

https://github.com/uber/deck.gl/blob/master/scripts/extract-types.js

However, I am not familiar the exact syntax of a flow type definition file, you would just need to help with implementing that.

For the rest of the API it is mainly the various methods of the Deck class, the View classes etc that would need manual flow type definition entries in your file.

Does this help?

@IgorWilbert IgorWilbert force-pushed the IgorWilbert:adds-flow-definitions branch from cb69c33 to d621dd4 Mar 2, 2019

@IgorWilbert

This comment has been minimized.

Copy link
Contributor Author

IgorWilbert commented Mar 2, 2019

Hi @ibgreen ! Thanks for the help. Sorry to bother you, but before proceeding I would like to know your thoughts about what I have done so far.
I attempted to translate part of the code from the runtime prop type system. I know that you're unfamiliar with flow type definition syntax, I just wanted to know your opinion in terms of file and code structure. I see that many definitions are repeated, so probably we could get rid of them and perhaps make just one big defaultProps declaration with unique types.
In lite.js I also tried to start the definitions for

import Mapbox from 'react-map-gl/src/mapbox/mapbox';
but I'll probably organize it better. What do you think?

@@ -0,0 +1,189 @@
//arc-layer
declare const defaultProps = {

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 4, 2019

Contributor

@IgorWilbert I would assume that:

  • You want to call this definition object ArcLayerPropTypes or similar, rather than a const.
  • You would want to declare the ArcLayer constructor as accepting this prop object type definition: declare function ArcLayer(props : ArcLayerPropTypes);
  • Ideally flow allows a prop object type declaration to inherit from another: declare ArcLayerPropTypes extends LayerPropTypes

But again I don't know enough about flow type declaration file syntax, perhaps the above is not possible.

@IgorWilbert IgorWilbert force-pushed the IgorWilbert:adds-flow-definitions branch from d621dd4 to d844378 Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.