[react-jss] Add flow typings #818

gomesalexandre · 2018-08-07T12:05:04Z

This is a work-in-progress PR, this first commit is pretty much just the work done before the switch to monorepo.

TODO:

100% flow coverage for react-jss
adding script for copying source files

HenriBeck · 2018-08-07T14:36:17Z

package.json

@@ -99,5 +99,8 @@
    "sinon": "4.5.0",
    "webpack": "^2.3.3",
    "zen-observable": "^0.6.0"
+  },
+  "dependencies": {
+    "flow-copy-source": "^2.0.2"


We don't copy the source files. We already do the step to use flow typings.

Fair enough, I didn't see the babel-plugin-transform-flow-strip-types babel plugin, removed

HenriBeck · 2018-08-07T14:36:53Z

packages/react-jss/.flowconfig

@@ -0,0 +1,11 @@
+[ignore]


This file shouldn't exist because there is a flow config in the root for all packages

HenriBeck · 2018-08-07T14:37:32Z

packages/react-jss/package.json

@@ -33,7 +33,8 @@
    "decorator"
  ],
  "scripts": {
-    "prepare": "node ../../scripts/prepare.js"
+	"prepare": "node ../../scripts/prepare.js",
+	"flow": "flow"


Again, we don't need to run flow here as it's being run from the root directory

HenriBeck · 2018-08-07T14:40:13Z

packages/react-jss/src/createHoc.js

-const getStyles = (stylesOrCreator, theme) => {
+type Props = {
+  classes: ?{},
+  innerRef?: () => {},


should be (comp: ComponentType<{}> | null) => void. You could make the component type more exact with moving the props into the generic types of createHOC

Used (comp: ComponentType<{}> | null) => void in the current implementation, will implement generics in the next PR, it's still a bit of overhead for me still

HenriBeck · 2018-08-07T14:41:00Z

packages/react-jss/src/createHoc.js

      // We remove previous dynamicSheet only after new one was created to avoid FOUC.
-      if (prevState.dynamicSheet !== this.state.dynamicSheet && prevState.dynamicSheet) {


&& prevState.dynamicSheet should not be removed!

Same diff reason, fixed

HenriBeck · 2018-08-07T14:43:42Z

packages/react-jss/src/types/index.js

+  RuleOptions,
+  JssStyle
+} from 'jss/lib/types'
+import RuleList from 'jss/lib/RuleList'


should be imported from jss as a type

HenriBeck · 2018-08-07T14:44:01Z

packages/react-jss/src/types/index.js

+  ToCssOptions,
+  RuleOptions,
+  JssStyle
+} from 'jss/lib/types'


All of those should be imported from jss

HenriBeck · 2018-08-07T14:44:37Z

packages/react-jss/src/types/index.js

+import RuleList from 'jss/lib/RuleList'
+import {create} from '../jss'
+
+export interface JssSheet {


No need to redefine the StyleSheet here. Just import it from jss as a type

HenriBeck · 2018-08-07T14:45:03Z

packages/react-jss/src/types/index.js

+    themeListener: any => any
+  },
+  inject?: Array<'classes' | 'themes' | 'sheet'>,
+  jss?: create,


Should be a Jss instance and not a function which creates an instance

HenriBeck · 2018-08-07T14:50:51Z

packages/react-jss/src/createHoc.js

+      const sheet = dynamicSheet || staticSheet
+      // $FlowFixMe defaultProps are not defined in React$Component
+      const defaultClasses = InnerComponent.defaultProps ? InnerComponent.defaultProps.classes : {}
+      const classes = {...defaultClasses, ...sheet.classes, ...userClasses}



Why did you change the logic of this code?

For now this is essentially a copy and paste from previous PR. Given the diff shows there are some changes (such as this one) that haven't been integrated in it, I'll take care of it top-priority, before working on anything else

HenriBeck · 2018-08-07T14:51:29Z

Also please add typings for the other files when you have time. Especially for the JssProvider

HenriBeck · 2018-08-08T12:55:01Z

packages/react-jss/src/createHoc.js

+          getDynamicStyles(styles)
+        )
+        staticSheet[dynamicStylesNs] = dynamicStyles
+      } else dynamicStyles = staticSheet[dynamicStylesNs]


Again, no logic changes here please

HenriBeck · 2018-08-08T12:55:48Z

packages/react-jss/src/createHoc.js

+  stylesOrCreator: StylesOrThemer,
+  InnerComponent: ComponentType<P>,
+  options: Options = {}
+): ComponentType<any> {


The returning component props should be typed exactly

HenriBeck · 2018-08-09T10:47:29Z

packages/react-jss/src/compose.js

@@ -1,3 +1,5 @@
+// @flow
+type Classes = {[string]: string}


Should be also imported from jss https://github.com/cssinjs/jss/blob/master/packages/jss/src/index.js#L33

HenriBeck · 2018-08-09T10:49:27Z

packages/react-jss/src/types/index.js

@@ -0,0 +1,19 @@
+// @flow
+import type {StyleSheetOptions} from 'jss'
+import jss from '../jss'


Import the Jss type also from jss

HenriBeck · 2018-08-09T10:49:45Z

packages/react-jss/src/types/index.js

+export type Styles = {[string]: {}}
+export type ThemerFn = (theme: Theme) => Styles
+export type StylesOrThemer = Styles | ThemerFn
+export type Classes<S> = {|


Should be also imported from jss https://github.com/cssinjs/jss/blob/master/packages/jss/src/index.js#L33

HenriBeck · 2018-08-09T10:58:56Z

Quick guideline on which props the HOC should accept:

All of the props of the injected Component
An additional innerRef prop which needs to be a function with the injected Component as an argument

I don't think we need to use the $Diff operator for props like classes because the user is actually allowed to overwrite those props.

gomesalexandre · 2018-08-09T14:01:45Z

@HenriBeck Do we really need to init options to empty object literal {} if not passed to createHOC? It produces a bit of flow errors because of l.85 i.e
const {theming = defaultTheming, inject, jss: optionsJss, ...sheetOptions} = options.

Then of course, accessing a property of undefined options object with no default would produce errors but that could be mitigated.

gomesalexandre · 2018-08-09T14:07:57Z

@HenriBeck Regarding the HOC, the returned component is stripped of the classes prop isn't it ? That's the logic behing the use of the $Diff util here. But in that case I can simply use an intersection type instead of a Diff type

HenriBeck · 2018-08-09T16:06:33Z

No, I was wrong about that before, see https://github.com/cssinjs/jss/blob/master/packages/react-jss/src/createHoc.js#L191

User classes override the from the stylesheet.

romelperez · 2018-08-09T21:12:07Z

.eslintrc

@@ -4,7 +4,8 @@
  "rules": {
    "import/no-unresolved": 0,
    "flowtype/define-flow-type": 1,
-    "flowtype/use-flow-type": 1
+    "flowtype/use-flow-type": 1,
+	"semi": ["error", "never"]


Should we use spaces instead of tabs?

I think this can be reverted anyway because prettier takes care of it. I actually write the code with semicolons and prettier removes them for me

But yes, definetly spaces

Using the indent rule would avoid such issue, I have automatic linting fixes on save in VSCode. Plus we would get errors thrown within the hook, should I add it ?

We also run prettier in the precommit git hook so it's unnecessary

…rops utility types

- Used OuterProps and InnerProps types - Fixed the props of the returning component

…s type

- Made most props optional - Only allow one child

HenriBeck · 2018-08-12T20:47:46Z

@kof types should be done

…egistry after component unmounts

kof · 2018-08-15T18:26:42Z

packages/react-jss/src/createHoc.js

@@ -57,41 +74,49 @@ let managersCounter = 0
 /**
 * Wrap a Component into a JSS Container Component.
 *
+ * IProps: Props of the InnerComponent.
+ * OProps: The Outer props the HOC accepts.
+ *
 * @param {Object|Function} stylesOrCreator


I think we can remove those jsdoc comments now

Will do some cleanup in another PR. Have some small fixes etc. in mind

kof · 2018-08-15T18:27:30Z

packages/react-jss/src/createHoc.js

@@ -57,41 +74,49 @@ let managersCounter = 0
 /**
 * Wrap a Component into a JSS Container Component.
 *
+ * IProps: Props of the InnerComponent.


InnerProps would be probably a better name, no?

already using InnerProps though, that's why I had to name it IProps 🤔

lets find a better convention, maybe InnerPropsType?

kof · 2018-08-15T18:30:13Z

packages/react-jss/src/JssProvider.js

 import PropTypes from 'prop-types'
+import type {Jss, generateClassName as GenerateClassNameType, SheetsRegistry} from 'jss'


why mapping from lower case to capital?

ohh, I think we need to rename it to upper case in the core package in the first place

Once it starts uppercase you don't need to add Type suffix either

kof · 2018-08-15T20:13:43Z

packages/react-jss/src/createHoc.js

@@ -176,6 +209,7 @@ export default (stylesOrCreator, InnerComponent, options = {}) => {
        })
      }

+      // $FlowFixMe InnerComponent can be class or stateless, the latter doesn't have a defaultProps property


it may have defaultProps property same way as class based

yeah but flow doesn't have defaultProps typed on the class nor the functional component https://github.com/facebook/flow/blob/master/lib/react.js#L99

weird, we should look for an issue there for explanation or raise one .... really weird

The explanation is in the comment above.

lol ... my day is over :)

kof · 2018-08-15T20:15:47Z

packages/react-jss/src/types.js

+  b768b78919504fba9de2c03545c5cd3a?: {[key: number]: SheetsManager},
+  '6fc570d6bd61383819d0f9e7407c452d': StyleSheetFactoryOptions & {disableStylesGeneration?: boolean}
+}
+export type OuterProps<IP, C> = IP & {innerRef: (instance: ElementRef<C> | null) => void}


Lets not use these abbreviations, they are hard to understand

kof · 2018-08-15T20:16:30Z

packages/react-jss/src/types.js

+export type OuterProps<IP, C> = IP & {innerRef: (instance: ElementRef<C> | null) => void}
+export type Styles = {[string]: {}}
+export type ThemerFn = (theme: Theme) => Styles
+export type StylesOrThemer = Styles | ThemerFn


ThemerFn -> ThemeCreator

ThemeCreator seems a little bit misleading

Maybe StylesCreator?

my bad, yes StylesCreator

* First commit * Fixes on createHOC and injectSheet * More fixes on createHOC and injectSheet * Better flow typings on createHOC * Moved types/index.js just to types.js and added InnerProps and OuterProps utility types * Simplified typings of injectSheet.js * Updated and fixed some typings in createHOC.js - Used OuterProps and InnerProps types - Fixed the props of the returning component * Removed unnecessary types in ns.js * Added flow flag to some untyped files * Exporting StyleSheetFactoryOptions now instead of StyleSheetOptions * Fix typing of getDisplayName * Added Context Type and updated Options to use StyleSheetFactoryOptions type * Moved types/index.js just to types.js and added * Improved Context Type * Updated createHoc to use Context type * Improved typing of the Theming option * Formatted code with prettier * Updated size snapshot and lockfile * Make options in injectSheet optional an default it to an empty object * Fixed types of the JssProvider props - Made most props optional - Only allow one child * Remove semi eslint rule * Removed unnecessary types * Remove unnecessary check * Updated a if statement and fixed removing dynamic sheet from global registry after component unmounts * Fixed naming of some type names to always start with an uppercase char * Renamed generic types of createHOC

First commit

b4b0523

HenriBeck reviewed Aug 7, 2018

View reviewed changes

HenriBeck requested changes Aug 7, 2018

View reviewed changes

Fixes on createHOC and injectSheet

e008910

HenriBeck requested changes Aug 8, 2018

View reviewed changes

More fixes on createHOC and injectSheet

bd5348a

HenriBeck reviewed Aug 9, 2018

View reviewed changes

romelperez reviewed Aug 9, 2018

View reviewed changes

gomesalexandre and others added 15 commits August 10, 2018 15:49

Better flow typings on createHOC

887652f

Moved types/index.js just to types.js and added InnerProps and OuterP…

3954fc5

…rops utility types

Simplified typings of injectSheet.js

a7cca18

Updated and fixed some typings in createHOC.js

b379272

- Used OuterProps and InnerProps types - Fixed the props of the returning component

Removed unnecessary types in ns.js

b1fb062

Added flow flag to some untyped files

1d69fce

Exporting StyleSheetFactoryOptions now instead of StyleSheetOptions

0c07422

Fix typing of getDisplayName

386f2e5

Added Context Type and updated Options to use StyleSheetFactoryOption…

839e1ae

…s type

Moved types/index.js just to types.js and added

fccbbf0

Improved Context Type

5f1c1e8

Updated createHoc to use Context type

d867aa2

Improved typing of the Theming option

539c3ed

Formatted code with prettier

bacb6bd

Updated size snapshot and lockfile

2b86b8d

Henri Beck added 3 commits August 12, 2018 17:55

Make options in injectSheet optional an default it to an empty object

ecdf147

Fixed types of the JssProvider props

4c16374

- Made most props optional - Only allow one child

Remove semi eslint rule

3ad6097

HenriBeck approved these changes Aug 12, 2018

View reviewed changes

HenriBeck requested a review from kof August 12, 2018 16:00

HenriBeck changed the title ~~WIP PR: Add react-jss flow typings~~ [react-jss] Add flow typings Aug 14, 2018

Henri Beck added 3 commits August 14, 2018 18:23

Removed unnecessary types

1a803f9

Remove unnecessary check

44d7dd5

Updated a if statement and fixed removing dynamic sheet from global r…

9da04d3

…egistry after component unmounts

kof reviewed Aug 15, 2018

View reviewed changes

Henri Beck added 3 commits August 15, 2018 21:16

Fixed naming of some type names to always start with an uppercase char

3d7d984

Renamed generic types of createHOC

fcd4345

Merge branch 'master' into react-jss-flow-typings

629d9f5

HenriBeck merged commit 1e9ba74 into cssinjs:master Aug 15, 2018

kof reviewed Aug 15, 2018

View reviewed changes

HenriBeck mentioned this pull request Oct 22, 2018

[react-jss] Add flowtype support #797

Closed

		// We remove previous dynamicSheet only after new one was created to avoid FOUC.
		if (prevState.dynamicSheet !== this.state.dynamicSheet && prevState.dynamicSheet) {

		import PropTypes from 'prop-types'
		import type {Jss, generateClassName as GenerateClassNameType, SheetsRegistry} from 'jss'

[react-jss] Add flow typings #818

[react-jss] Add flow typings #818

Conversation

gomesalexandre commented Aug 7, 2018

Choose a reason for hiding this comment

gomesalexandre Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HenriBeck commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HenriBeck commented Aug 9, 2018

gomesalexandre commented Aug 9, 2018

gomesalexandre commented Aug 9, 2018 • edited Loading

HenriBeck commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gomesalexandre Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HenriBeck commented Aug 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HenriBeck Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gomesalexandre Aug 8, 2018 •

edited

Loading

gomesalexandre commented Aug 9, 2018 •

edited

Loading

gomesalexandre Aug 10, 2018 •

edited

Loading

HenriBeck Aug 15, 2018 •

edited

Loading