[Flow] Define Flow types for style and stylesheet objects, more checking #8882

ide · 2016-07-19T07:46:33Z

Define a Styles type for the object that gets passed into StyleSheet.create
Define a StyleSheet type that is returned from StyleSheet.create
Clean up the type declarations in StyleSheet.create

Test Plan: flow

ghost · 2016-07-19T07:47:33Z

By analyzing the blame information on this pull request, we identified @sahrens and @prometheansacrifice to be potential reviewers.

ide · 2016-07-19T18:51:22Z

cc @vjeux @brentvatne

vjeux · 2016-07-19T18:55:07Z

@yungsters can you take a look?

vjeux · 2016-07-19T18:55:51Z

Oh this is actually not adding any new strictness, I can just ship it, thanks

vjeux · 2016-07-19T18:55:54Z

@facebook-github-bot shipit

facebook-github-bot · 2016-07-19T18:56:25Z

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

vjeux · 2016-07-19T19:45:00Z

Can you rebase?

- Define a Styles type for the object that gets passed into StyleSheet.create - Define a StyleSheet type that is returned from StyleSheet.create - Clean up the type declarations in StyleSheet.create Test Plan: `flow`

ghost · 2016-07-20T08:48:08Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

satya164 · 2016-07-20T10:23:30Z

@facebook-github-bot shipit

satya164 · 2016-07-20T10:24:59Z

Libraries/StyleSheet/StyleSheet.js

@@ -17,6 +17,9 @@ var StyleSheetValidation = require('StyleSheetValidation');

 var flatten = require('flattenStyle');

+export type Styles = {[key: string]: Object};


Could be {[key: string]: {[key: string]: any} } right?

Isn't {[key: string]: any} the same as Object?

@ide Not exactly sure how Flow works here. Do arrays also count as objects? If not then they are same :)

Yep they are different:

@ide Cool then

ghost · 2016-07-20T10:25:38Z

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

vjeux · 2016-07-20T13:14:11Z

It finds some more internal bad code. Someone needs to fix them internally first.

ghost · 2016-07-20T23:48:59Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

javache · 2016-08-04T12:44:30Z

For some reason, internally this is failing with

react-native-github/Libraries/StyleSheet/StyleSheet.js:20
 20: export type Styles = {[key: string]: Object};
                                          ^^^^^^ object type. This type is incompatible with
 21: export type StyleSheet<S: Styles> = {[key: $Keys<S>]: number};
                                                           ^^^^^^ number

react-native-github/Libraries/StyleSheet/StyleSheet.js:21
 21: export type StyleSheet<S: Styles> = {[key: $Keys<S>]: number};
                                                           ^^^^^^ number. This type is incompatible with
 20: export type Styles = {[key: string]: Object};
                                          ^^^^^^ object type


Found 2 errors

ide · 2016-08-12T20:54:23Z

That error doesn't make sense to me, reading the code and running flow (v0.30) myself. Perhaps a different version of Flow? Will try again.

@facebook-github-bot shipit

ghost · 2016-08-12T20:55:14Z

I will not rubber stamp and land your change for you @ide! I can import it for you and you can get your change reviewed by someone though :)

vjeux · 2016-08-12T21:06:42Z

@facebook-github-bot shipit

ghost · 2016-08-12T21:07:54Z

Thanks for importing.If you are an FB employee go to Phabricator to review internal test results.

ghost · 2016-08-12T23:48:10Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

gabelevi · 2016-08-13T01:00:00Z

It's possible that improving the types like this exposes mistakes in user code. If you need help I can assist in the merge on Monday

gabelevi · 2016-08-15T16:34:48Z

Ok, fixed this up and landing it!

The flow error wasn't occurring within the react-native code, but rather in some code that uses react-native. Specifically, it was a jest mock that mocked StyleSheet.create() with an incorrect type. The error message was unfortunate in that it didn't point out the location of the mistake until you ran flow check --traces 10

ide · 2016-08-15T18:35:52Z

@gabelevi thanks so much for looking into the issue and merging this =)

Summary: - Define a Styles type for the object that gets passed into StyleSheet.create - Define a StyleSheet type that is returned from StyleSheet.create - Clean up the type declarations in StyleSheet.create Closes facebook#8882 Differential Revision: D3587964 Pulled By: gabelevi fbshipit-source-id: 629e0176484436848be69b1417638e1200a3669a

ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jul 19, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 19, 2016

ide force-pushed the flow-with-stylesheet-create branch from 276ece9 to 23d77dd Compare July 19, 2016 21:52

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 19, 2016

ghost added Import Failed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Jul 20, 2016

satya164 reviewed Jul 20, 2016
View reviewed changes

ghost added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jul 20, 2016

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 20, 2016

ghost added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jul 20, 2016

ghost added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Jul 20, 2016

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 12, 2016

ghost added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Aug 12, 2016

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 12, 2016

ghost added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Aug 12, 2016

ghost closed this in 8eed600 Aug 15, 2016

ide deleted the flow-with-stylesheet-create branch August 15, 2016 18:34

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Flow] Define Flow types for style and stylesheet objects, more checking #8882

[Flow] Define Flow types for style and stylesheet objects, more checking #8882

ide commented Jul 19, 2016

ghost commented Jul 19, 2016

ide commented Jul 19, 2016

vjeux commented Jul 19, 2016

vjeux commented Jul 19, 2016

vjeux commented Jul 19, 2016

facebook-github-bot commented Jul 19, 2016

vjeux commented Jul 19, 2016

ghost commented Jul 20, 2016

satya164 commented Jul 20, 2016

satya164 Jul 20, 2016

ide Jul 20, 2016

satya164 Jul 20, 2016

ide Jul 20, 2016 •

edited

Loading

satya164 Jul 20, 2016

ghost commented Jul 20, 2016

vjeux commented Jul 20, 2016

ghost commented Jul 20, 2016

javache commented Aug 4, 2016

ide commented Aug 12, 2016

ghost commented Aug 12, 2016

vjeux commented Aug 12, 2016

ghost commented Aug 12, 2016

ghost commented Aug 12, 2016

gabelevi commented Aug 13, 2016

gabelevi commented Aug 15, 2016

ide commented Aug 15, 2016

		@@ -17,6 +17,9 @@ var StyleSheetValidation = require('StyleSheetValidation');

		var flatten = require('flattenStyle');

		export type Styles = {[key: string]: Object};

[Flow] Define Flow types for style and stylesheet objects, more checking #8882

[Flow] Define Flow types for style and stylesheet objects, more checking #8882

Conversation

ide commented Jul 19, 2016

ghost commented Jul 19, 2016

ide commented Jul 19, 2016

vjeux commented Jul 19, 2016

vjeux commented Jul 19, 2016

vjeux commented Jul 19, 2016

facebook-github-bot commented Jul 19, 2016

vjeux commented Jul 19, 2016

ghost commented Jul 20, 2016

satya164 commented Jul 20, 2016

satya164 Jul 20, 2016

Choose a reason for hiding this comment

ide Jul 20, 2016

Choose a reason for hiding this comment

satya164 Jul 20, 2016

Choose a reason for hiding this comment

ide Jul 20, 2016 • edited Loading

Choose a reason for hiding this comment

satya164 Jul 20, 2016

Choose a reason for hiding this comment

ghost commented Jul 20, 2016

vjeux commented Jul 20, 2016

ghost commented Jul 20, 2016

javache commented Aug 4, 2016

ide commented Aug 12, 2016

ghost commented Aug 12, 2016

vjeux commented Aug 12, 2016

ghost commented Aug 12, 2016

ghost commented Aug 12, 2016

gabelevi commented Aug 13, 2016

gabelevi commented Aug 15, 2016

ide commented Aug 15, 2016

ide Jul 20, 2016 •

edited

Loading