-
Notifications
You must be signed in to change notification settings - Fork 43
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
Flow Type Support #56
Comments
Cool! The only thing I see is a minor enhancement. For the Objective-C backend, we generate a comment next to the integer enum types: typedef NS_ENUM(NSInteger, PIPinAttributionImageType) {
PIPinAttributionImageTypeUnset /* unset */,
PIPinAttributionImageTypeBoard /* board */,
PIPinAttributionImageTypeInterest /* interest */,
PIPinAttributionImageTypeUser /* user */
}; I think this information especially important in JavaScript since the Flow definition doesn't name the numbers (although it could). Perhaps we at least include the comments like we do in Objective-C: export type PinAvailabilityType =
| 1 /* in_stock */
| 2 /* out_of_stock */
| 3 /* preorder */
| 4 /* unavailable */; |
For
My opinion: we should require that descriptions are unique and short (maybe introduce another |
@bkase: Added the description to the integer enums. @bradencanderson: The enum types are based on how it's defined in the PDK. We support integer and string enums. E.g. This is a string enum:
And that's an integer enum:
|
Integer enums seem problematic because they're structurally typed -- |
Merged |
We are planning to add Flow type support to plank and and here are some initial thoughts around it.
The initial idea was to provide a more extended JS integration with immutable models backed by Immutable.js. A partial implementation was already on a side branch too, but as Immutable.js is pretty hefty in size as well as would introduce a certain convention to use Immutable.js up front, we decided to take a step back for the first pass and start with adding Flow support to plank.
Pull Request
A pull request that implements this issue as currently described is ready: #57
Example
For a first example what the following PDT definition of an extensive representation of a Pin type is provided as well as the created Flow type definition below.
PDT
Flow
Some notes
Let's pick out some interesting pieces from the example above and provide bit more details.
Type alias
For every plank type an exported Flow type alias with the name
TitleType
will be created.Property variance (read-only)
Currently all properties are defined as covariant (read-only), declared by the plus symbol in front of the property name.
Optional properties
As it's currently not possible to know for sure, that properties are included within the API response we declare the properties as optional. Furthermore, if the property is included in the API response we don't know for sure if a valid value or null was received. Therefore the type definition of a property is always declared as optional.
Primitive type properties
For types like integer or strings, equivalent primitive types like number and string are used.
Object as maps properties
For object types that act like a map, like the
counts
property from above, we use a special kind of property, called an "indexer property".Format type properties
For specific format types, we are providing pre-defined types which are defined in a specific runtime file. In case of the
date
anduri
type the representation is just a string for now:Reference properties
If references to other types are defined within the PDT, the referenced type will be imported and the property will be annotated with the reference type.
Enum and ADT properties
We also have support for enums and ADTs, which are not present in the example above. Examples for both of them would look like the following:
ADTs
PDT
Flow
Enums
PDT
Flow
Feedback welcome
As we would like to provide the most optimal integration that would suit most of the needs, with this issue we would like to start gathering feedback from the community about our current approach and were we should heading to.
cc @rahul-malik @bkase @chrislloyd @bradencanderson
The text was updated successfully, but these errors were encountered: