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

chore: export flow types #94

Merged
merged 14 commits into from
Aug 2, 2018
13 changes: 11 additions & 2 deletions .npmignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,10 @@
coverage
dist
e2e
flow-typed
node_modules
rfcs
scripts
src
flow-typed

commitlint.config.js
docker-compose.yml
Expand All @@ -15,3 +14,13 @@ jest-e2e.config.js
jest.config.js
rollup.config_e2e.js
rollup.config.js

# The parts below are due to flow type
*.test.js
*.setup.js
*.stories.js
stories.js
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

*.stories.js
stories.js

Can these be combined into just stories.js?

*.test.js.snap
test
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Can we combine these into just test?

__mocks__
e2e.js
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,13 @@
},
"scripts": {
"lint": "eslint ./",
"flow": "flow --show-all-errors",
Copy link
Collaborator Author

@ardok ardok Jul 29, 2018

Choose a reason for hiding this comment

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

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

"commitmsg": "commitlint -e $GIT_PARAMS",
"test": "yarn lint && yarn unit-test",
"storybook": "start-storybook -p 6006",
"build-storybook": "build-storybook -o docs",
"precommit": "pretty-quick --staged && yarn run test",
"build": "rollup -c rollup.config.js",
"build": "rm -rf dist && rollup -c rollup.config.js",
"unit-test": "yarn jest --coverage",
"e2e": "node ./e2e/e2e.js",
"build-e2e": "rollup -c rollup.config_e2e.js",
Expand Down
3 changes: 3 additions & 0 deletions rollup.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

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

let's not use 'uber' prefix here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okie


function getComponents() {
return fs.readdirSync('./src/components').filter(filename => {
const {ext, name} = path.parse(filename);
Expand Down Expand Up @@ -70,6 +72,7 @@ function getSharedConfig({filePath, name}) {
'node_modules/create-react-context/index.js': ['createReactContext'],
},
}),
uberFlowEntry(),
],
};
}
Expand Down
73 changes: 73 additions & 0 deletions scripts/rollup-plugin-flow-entry.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/* eslint-disable flowtype/require-valid-file-annotation */
Copy link
Contributor

Choose a reason for hiding this comment

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

same here: // @flow ?

import fs from 'fs';
import path from 'path';

/*
We can consider this as well:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will add more

https://github.com/RichieAHB/rollup-plugin-flow-defs/blob/master/src/index.js

This code is inspired by that npm package anyway.
*/

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will update the comments

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');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, will add more comments on it

const matched = filePath.match(regex);
if (matched.length > 0) {
// Check whether it's only one `..`
if (matched.length === 1) {
// Only 1, replace it with `./`
filePath = filePath.replace('../', './');
} else {
// Must be more than 1
filePath = filePath.replace('../', '');
}
}
return `// @flow
// $FlowFixMe path is assuming that it's already in root directory
export * from '${filePath}';`;
}

export default function uberFlowEntry() {
let input;
return {
name: '@uber/rollup-plugin-flow-entry',
options(opts) {
input = opts.input;
},
async generateBundle(opts) {
const {file = ''} = opts || {};

// Create file path if does not exist
await new Promise(resolve => {
const dirname = path.dirname(file);
if (!fs.existsSync(dirname)) {
fs.mkdirSync(dirname);
}
resolve('ok');
});

await new Promise((resolve, reject) => {
/*
path.dirname(input); // src/components/popover
path.basename(input); // index.js
path.dirname(file); // this is output, `dist/popover`
*/
const relativePath = path.join(
path.relative(path.dirname(file), path.dirname(input)),
path.basename(input),
);
fs.writeFile(`${file}.flow`, getFlowFileContent(relativePath), err => {
if (err) {
reject(err);
}
resolve('ok');
});
});
},
};
}
3 changes: 3 additions & 0 deletions src/components/checkbox/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,6 @@ export {
Label as StyledLabel,
Input as StyledInput,
} from './styled-components';

// Flow
export * from './types';
3 changes: 3 additions & 0 deletions src/components/input/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,6 @@ export {
Caption as StyledCaption,
} from './styled-components';
export {STATE_CHANGE_TYPE, ADJOINED, SIZE} from './constants';

// Flow
export * from './types';
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to export these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

3 changes: 3 additions & 0 deletions src/components/popover/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,6 @@ export {
Inner as StyledInner,
Padding as StyledPadding,
} from './styled-components';

// Flow
export * from './types';
3 changes: 3 additions & 0 deletions src/components/tooltip/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,6 @@ export {
Body as StyledBody,
Inner as StyledInner,
} from './styled-components';

// Flow
export * from './types';
1 change: 1 addition & 0 deletions src/helpers/overrides.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// @flow
import * as React from 'react';

export type OverrideT<T> =
| {
Expand Down