-
Notifications
You must be signed in to change notification settings - Fork 828
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
Changes from 3 commits
e4b7229
3dd9d65
f6dc491
6f9e24f
6d7c74b
c679b2d
4d869f0
935661a
243eb12
7b16589
d4d4bc1
8fe81f0
0b7e481
49a356e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,11 +2,10 @@ | |
coverage | ||
dist | ||
e2e | ||
flow-typed | ||
node_modules | ||
rfcs | ||
scripts | ||
src | ||
flow-typed | ||
|
||
commitlint.config.js | ||
docker-compose.yml | ||
|
@@ -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 | ||
*.test.js.snap | ||
test | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can we combine these into just |
||
__mocks__ | ||
e2e.js |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,12 +18,13 @@ | |
}, | ||
"scripts": { | ||
"lint": "eslint ./", | ||
"flow": "flow --show-all-errors", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's not use 'uber' prefix here There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -70,6 +72,7 @@ function getSharedConfig({filePath, name}) { | |
'node_modules/create-react-context/index.js': ['createReactContext'], | ||
}, | ||
}), | ||
uberFlowEntry(), | ||
], | ||
}; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
/* eslint-disable flowtype/require-valid-file-annotation */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here: |
||
import fs from 'fs'; | ||
import path from 'path'; | ||
|
||
/* | ||
We can consider this as well: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'); | ||
}); | ||
}); | ||
}, | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,3 +13,6 @@ export { | |
Caption as StyledCaption, | ||
} from './styled-components'; | ||
export {STATE_CHANGE_TYPE, ADJOINED, SIZE} from './constants'; | ||
|
||
// Flow | ||
export * from './types'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need to export these? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So that you could do: |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
// @flow | ||
import * as React from 'react'; | ||
|
||
export type OverrideT<T> = | ||
| { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these be combined into just
stories.js
?