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

Add Flow Type Support #57

Merged
merged 2 commits into from
Jun 16, 2017
Merged

Add Flow Type Support #57

merged 2 commits into from
Jun 16, 2017

Conversation

maicki
Copy link
Contributor

@maicki maicki commented May 30, 2017

Further details #56

TODOs:

  • Add flow type verification step for integration test
  • Add non nullable and non optional id field

Follow Up (No blocker):

  • Add documentation for flow support
    • Update CLI help command
    • Add documentation for the website
    • Change tagline of project
  • Add dynamic spaces support (JS usually uses 2 spaces) Add custom indentation support #61
  • Generate an index.js file that exports all the flow types

@maicki maicki mentioned this pull request May 30, 2017
// DO NOT EDIT - EDITS WILL BE OVERWRITTEN
// @generated
//
// @flow

Choose a reason for hiding this comment

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

Does this work? I think Flow only looks at the first 10 tokens in a file by default. (Might want to move // @flow to the top just in case)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for reference, what does @flow do for us?

Choose a reason for hiding this comment

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

// @flow tells Flow to typecheck the file. Otherwise it will ignore it and assume everything coming out of it is any.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my testing it works but I will move it to the top.

import type { PlankDate, PlankURI } from "./runtime.flow.js";


export type ImageType = {

Choose a reason for hiding this comment

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

should this be an exact object type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed!


export type PinType = {
+note?: string | null,
+media?: { [string]: string } | null,
Copy link

@bradencanderson bradencanderson May 30, 2017

Choose a reason for hiding this comment

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

The contents of media, counts, etc should be read-only as well: +media?: { +[string]: string }.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Read only?

Choose a reason for hiding this comment

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

Yes, thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@bradencanderson
Copy link

Can you also generate an index.js file that exports all these types? That way, we can do

import type { BoardType, ImageType } from 'plank-schemas';

instead of

import type { BoardType } from 'plank-schemas/BoardType';
import type { ImageType } from 'plank-schemas/ImageType';

Also, make sure this index.js file only exports flowtypes -- we don't want to increase bundle size.

@bradencanderson
Copy link

Try to use import type { DomainType } from "./DomainType.js"; instead of import type DomainType from "./DomainType.js";.

A few more nits:

  • 2-space indents. This will make it easier for JS developers to dive in and read the code without messing with their editor configs.
  • Single quotes('), for the same reason.

If you want, you can try running prettier on your output.

import type { ImageType } from './ImageType.js';

export type BoardType = $Shape<{|
+name: ?string,

Choose a reason for hiding this comment

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

should there be an id field on all these objects?

Choose a reason for hiding this comment

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

update: I think there has to be, since a lot of our JS code depends on id being a field on pins, boards, users, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just pushed a commit that added support for an id field.

Copy link
Collaborator

@rahul-malik rahul-malik left a comment

Choose a reason for hiding this comment

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

Do we need to update any of the CLI help command?

@@ -6,6 +6,9 @@ JSON_FILES=`ls -d Examples/PDK/*.json`
# Generate Objective-C models
.build/debug/plank --output_dir=Examples/Cocoa/Sources/objc/ $JSON_FILES

#Generate flow types for models
.build/debug/plank --lang flow --output_dir=Examples/JS/flow/ $JSON_FILES
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there anything we can run to validate these are correct flow definitions since they are not compiled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can add a .flowconfig to the JS repo and run flow-bin CLI command over the output. We need to check if flow-bin is available globally on the dev's machine though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just did that. I added plain .flowconfig file for now to the example folder and run flow on the directory if flow is globally installed.

@maicki
Copy link
Contributor Author

maicki commented Jun 14, 2017

@rahul-malik We could add some comment to the --lang command which languages are supported e.g.:

--lang - Comma separated list of target language(s) for generating code. Default: objc - Supported: objc, flow

@levi
Copy link
Contributor

levi commented Jun 14, 2017

Before you land this, could you add documentation for the website? @rmalik we will need to change the tag line of the project whenever an official release is cut.

@maicki
Copy link
Contributor Author

maicki commented Jun 15, 2017

@rahul-malik Ready for next review.

As we talked about yesterday we should treat the flow support as experimental for now. Therefore I moved adding documentation as well as some other non blocking improvements like creating an index.js file or adding support for variable spaces support to next steps.

@ghost
Copy link

ghost commented Jun 15, 2017

🚫 CI failed with log

@ghost
Copy link

ghost commented Jun 15, 2017

🚫 CI failed with log

@ghost
Copy link

ghost commented Jun 15, 2017

2 Warnings
⚠️ This is a big PR, please consider splitting it up to ease code review.
⚠️ Any source code changes should have an entry in CHANGELOG.md or have #trivial in their title.

Generated by 🚫 Danger

@rahul-malik
Copy link
Collaborator

@maicki - Did you want to resolve the 2-space implementation within this patch or follow up?

@maicki
Copy link
Contributor Author

maicki commented Jun 16, 2017

@rahul-malik I'm currently working on a separate branch that will integrate dynamic indentation support to plank and I will get up a PR soon. In my opinion for this PR it's not necessary and would just bloat up the diff.

@maicki
Copy link
Contributor Author

maicki commented Jun 16, 2017

@rahul-malik PR for adding custom indentation support: #61

@rahul-malik rahul-malik merged commit 12db0da into pinterest:master Jun 16, 2017
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