chore: export flow types #94

ardok · 2018-07-29T02:05:42Z

Resolves: https://github.com/uber-web/baseui/issues/41

In order to export flow types, we need to include the original files that have the flow definition.
What we're going to do is to add export * from './types.js' from each components.
Then, we will add an accompanying files inside our dist directory (using a rollup plugin) that will end with .js.flow.
The content of the file will only be something like:

// @flow
export * from './src/index.js';

Then, we'll use flow-copy-source on our src directory by excluding some files using glob patterns.
And voila.

Alternatives

There's flow gen-flow-files, but it's still "experimental" and people seem to have issues with it.
facebook/flow#5519

Have tried it for baseui, and we also faced more issues:
facebook/flow#6171
facebook/flow#3281

Samples

// @flow
import {StatefulInput} from '@uber/baseui/input';
<StatefulInput onFocus={2} />

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/App.js:21:33

Cannot create StatefulInput element because number [1] is incompatible with function type [2] in property onFocus.

     src/App.js
     18│         <p className="App-intro">
     19│           To get started, edit <code>src/App.js</code> and save to reload.
     20│         </p>
 [1] 21│         <StatefulInput onFocus={2} />
     22│       </div>
     23│     );
     24│   }

     node_modules/@uber/baseui/src/components/input/types.js
 [2] 76│   onFocus: (e: SyntheticFocusEvent<HTMLInputElement>) => void,

// @flow
import type {BaseInputPropsT} from '@uber/baseui/input';
const test: BaseInputPropsT = {
  hello: 2,
};

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/App.js:9:31

Cannot assign object literal to test because:
 • property adjoined is missing in object literal [1] but exists in BaseInputPropsT [2].
 • property autoFocus is missing in object literal [1] but exists in BaseInputPropsT [2].
 • property disabled is missing in object literal [1] but exists in BaseInputPropsT [2].
 • property error is missing in object literal [1] but exists in BaseInputPropsT [2].
 • property id is missing in object literal [1] but exists in BaseInputPropsT [2].
 • property inputRef is missing in object literal [1] but exists in BaseInputPropsT [2].
 • property onBlur is missing in object literal [1] but exists in BaseInputPropsT [2].
 • property onChange is missing in object literal [1] but exists in BaseInputPropsT [2].
 • property onFocus is missing in object literal [1] but exists in BaseInputPropsT [2].
 • property overrides is missing in object literal [1] but exists in BaseInputPropsT [2].
 • property placeholder is missing in object literal [1] but exists in BaseInputPropsT [2].
 • property required is missing in object literal [1] but exists in BaseInputPropsT [2].
 • property size is missing in object literal [1] but exists in BaseInputPropsT [2].
 • property type is missing in object literal [1] but exists in BaseInputPropsT [2].
 • property value is missing in object literal [1] but exists in BaseInputPropsT [2].

         6│ import {StatefulInput} from '@uber/baseui/input';
         7│ import type {BaseInputPropsT} from '@uber/baseui/input';
         8│
 [2][1]  9│ const test: BaseInputPropsT = {
        10│   hello: 2,
        11│ };
        12│
        13│ class App extends Component<{}> {
        14│   render() {

Test

There might be other ways to test this, but this is how I test it:

Make these changes in package.json of baseui:

  "files": ["dist"],
  "main": "dist/baseui.js",
  "jsnext:main": "dist/baseui.es.js",
  "module": "dist/baseui.es.js",

At baseui root dir: yarn build
Remove package.json under dist
npm pack (npm pack will produce a tgz file).
Go to another project, call it foo, do npm install --save path/to/tgz/file
Go into foo/node_modules/@uber/baseui and copy the content of dist into the root of foo/node_modules/@uber/baseui.
Feel free to remove the dist directory.
./node_modules/.bin/flow stop && ./node_modules/.bin/flow on your project root

ardok · 2018-07-29T02:20:45Z

package.json

@@ -18,6 +18,7 @@
  },
  "scripts": {
    "lint": "eslint ./",
+    "flow": "flow --show-all-errors",


This should be run on commit hook as well and be addressed in another PR

ardok · 2018-07-29T02:21:16Z

scripts/prepublish.sh

@@ -6,4 +6,5 @@ fi;
 # When Buildkite publishes to npm, the published files are available in the root
 # directory, which allows for a clean include or require of sub-modules.
 yarn build
+node ./scripts/prepublish-flow.js


I'm not sure whether this would work in CI?

Come to think of it, I could just add $FlowFixMe and be done with it during rollup.
So we might end up not needing this prepublish-flow.js

ardok · 2018-07-29T02:22:25Z

.npmignore

+*.test.js
+*.setup.js
+*.stories.js
+stories.js


*.stories.js stories.js

Can these be combined into just stories.js?

ardok · 2018-07-29T02:22:57Z

.npmignore

+*.stories.js
+stories.js
+*.test.js.snap
+test


*.test.js *.test.js.snap test

Can we combine these into just test?

ardok · 2018-07-29T06:53:02Z

I'm not sure how to fix this flowtype from input:

export function getComponent<T>(
  override: ?OverrideT<T>,
  defaultComponent: React.ComponentType<T>,
): React.ComponentType<T> {
  if (override && typeof override === 'object') {
    return override.component || defaultComponent;
  }
  return override || defaultComponent;
}

DianaSuvorova · 2018-07-30T16:43:16Z

@ardok, rollup plugin you wrote looks interesting. Can you help me understand if the same result would be achieved if we use something like flow-copy-source in our prepublish.sh script instead?

nadiia · 2018-07-30T17:14:32Z

rollup.config.js

@@ -9,6 +9,8 @@ import commonjs from 'rollup-plugin-commonjs';
 import fs from 'fs';
 import path from 'path';

+import uberFlowEntry from './scripts/rollup-plugin-flow-entry';


let's not use 'uber' prefix here

ardok · 2018-07-30T17:33:39Z

@DianaSuvorova Actually, we should use that flow-copy-source to make all the src file ends with .js.flow.
But we would still need the plugin to produce an entry file.

DianaSuvorova · 2018-07-30T17:45:28Z

@ardok

But we would still need the plugin to produce an entry file.

What do you mean by an entry file?

ardok · 2018-07-30T18:14:31Z

@DianaSuvorova

// baseui.js.flow
// @flow
// $FlowFixMe path is assuming that it's already in root directory
export * from './src/index.js';

// input/index.js.flow
// @flow
// $FlowFixMe path is assuming that it's already in root directory
export * from '../src/components/input/index.js';

// etc...

ardok · 2018-07-30T18:25:58Z

rollup.config.js

@@ -70,6 +72,8 @@ function getSharedConfig({filePath, name}) {
          'node_modules/create-react-context/index.js': ['createReactContext'],
        },
      }),
+      flowEntry(),
+      // flowCopySrc(),


remove this

…rt-flow

ardok · 2018-07-31T22:11:16Z

package.json

+  "files": ["dist"],
+  "main": "dist/baseui.js",
+  "jsnext:main": "dist/baseui.es.js",
+  "module": "dist/baseui.es.js",


Ay, remove these

DianaSuvorova

Great job! Left you some comments mostly questions. so sad gen-flow-files is so buggy that we have to do this.

DianaSuvorova · 2018-08-01T23:33:58Z

scripts/flow-copy-src.js

@@ -0,0 +1,19 @@
+/* eslint-disable flowtype/require-valid-file-annotation */


why don't you just add // @flow it shouldn't hurt

Actually, yeah, I'll try that

DianaSuvorova · 2018-08-01T23:35:55Z

scripts/rollup-plugin-flow-entry.js

@@ -0,0 +1,73 @@
+/* eslint-disable flowtype/require-valid-file-annotation */


same here: // @flow ?

DianaSuvorova · 2018-08-01T23:36:54Z

scripts/rollup-plugin-flow-entry.js

+import path from 'path';
+
+/*
+We can consider this as well:


well, if you are adding this comment, can you explain why we didn't end up using it?

Will add more

DianaSuvorova · 2018-08-01T23:38:26Z

scripts/rollup-plugin-flow-entry.js

+
+function getFlowFileContent(filePath) {
+  /*
+  Since we're going to copy `dist` dictionary into root in our publish step,


it is not the case anymore. We are not copying dist into root. we are distributing dist instead.

Will update the comments

DianaSuvorova · 2018-08-01T23:39:08Z

scripts/rollup-plugin-flow-entry.js

+  we will need to remove one level of the path.
+  Hence, instead of `../src` and `../../src`, they will become `./src` and `../src` respectively.
+  */
+  const regex = new RegExp(/\.\.\//, 'g');


can you explain what this regex matches. I guess some descriptive name instead of regex would do.

Hmm, will add more comments on it

DianaSuvorova · 2018-08-01T23:47:56Z

src/components/input/index.js

@@ -13,3 +13,6 @@ export {
  Caption as StyledCaption,
 } from './styled-components';
 export {STATE_CHANGE_TYPE, ADJOINED, SIZE} from './constants';
+
+// Flow
+export * from './types';


why do we need to export these?

So that you could do:
import type {BaseInputPropsT} from '@uber/baseui/input

…rt-flow

DianaSuvorova · 2018-08-02T01:13:38Z

Looks good to me

* chore: export flow type * chore: simply flow export * chore: add comment * chore: use .js.flow files * chore: rebase with master * chore: rebase with master * chore: remove flow from scripts * chore: remove flowfixme * chore: go away unpm * chore: undo package.json changes on files and main stuff * chore: address comments * fix: flow error

chore: export flow type

e4b7229

ardok requested a review from gergelyke July 29, 2018 02:06

ardok changed the title ~~chore: export flow type~~ chore: export flow types Jul 29, 2018

ardok commented Jul 29, 2018

View reviewed changes

chore: simply flow export

3dd9d65

chore: add comment

f6dc491

nadiia reviewed Jul 30, 2018

View reviewed changes

chore: use .js.flow files

6f9e24f

ardok commented Jul 30, 2018

View reviewed changes

chore: rebase with master

6d7c74b

schnerd mentioned this pull request Jul 31, 2018

chore: fix flow types for overrides, popover #98

Merged

ardok added 5 commits July 31, 2018 11:27

chore: rebase with master

c679b2d

chore: remove flow from scripts

4d869f0

Merge branch 'master' of https://github.com/uber-web/baseui into expo…

935661a

…rt-flow

chore: remove flowfixme

243eb12

chore: go away unpm

7b16589

ardok commented Jul 31, 2018

View reviewed changes

chore: undo package.json changes on files and main stuff

d4d4bc1

ardok requested a review from DianaSuvorova August 1, 2018 22:35

DianaSuvorova reviewed Aug 1, 2018

View reviewed changes

ardok added 2 commits August 1, 2018 16:56

Merge branch 'master' of https://github.com/uber-web/baseui into expo…

8fe81f0

…rt-flow

chore: address comments

0b7e481

DianaSuvorova approved these changes Aug 2, 2018

View reviewed changes

fix: flow error

49a356e

ardok merged commit f7b3456 into master Aug 2, 2018

abmai deleted the export-flow branch August 13, 2018 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: export flow types #94

chore: export flow types #94

ardok commented Jul 29, 2018 •

edited

Loading

ardok Jul 29, 2018 •

edited

Loading

ardok Jul 29, 2018

ardok Jul 29, 2018

ardok Jul 29, 2018

ardok Jul 29, 2018

ardok commented Jul 29, 2018

DianaSuvorova commented Jul 30, 2018

nadiia Jul 30, 2018

ardok Jul 30, 2018

ardok commented Jul 30, 2018

DianaSuvorova commented Jul 30, 2018

ardok commented Jul 30, 2018

ardok Jul 30, 2018

ardok Jul 31, 2018

DianaSuvorova left a comment

DianaSuvorova Aug 1, 2018

ardok Aug 1, 2018

DianaSuvorova Aug 1, 2018

DianaSuvorova Aug 1, 2018

ardok Aug 1, 2018

DianaSuvorova Aug 1, 2018

ardok Aug 1, 2018

DianaSuvorova Aug 1, 2018

ardok Aug 1, 2018

DianaSuvorova Aug 1, 2018

ardok Aug 1, 2018

DianaSuvorova commented Aug 2, 2018

		@@ -0,0 +1,19 @@
		/* eslint-disable flowtype/require-valid-file-annotation */

		@@ -0,0 +1,73 @@
		/* eslint-disable flowtype/require-valid-file-annotation */

chore: export flow types #94

chore: export flow types #94

Conversation

ardok commented Jul 29, 2018 • edited Loading

Alternatives

Samples

Test

ardok Jul 29, 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

ardok commented Jul 29, 2018

DianaSuvorova commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ardok commented Jul 30, 2018

DianaSuvorova commented Jul 30, 2018

ardok commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DianaSuvorova left a 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

DianaSuvorova commented Aug 2, 2018

ardok commented Jul 29, 2018 •

edited

Loading

ardok Jul 29, 2018 •

edited

Loading