feat: typescript infrastructure and few components conversion #1797
Conversation
|
Verified that @priyajeet has signed the CLA. Thanks for the pull request! |
| "flow.useNPMPackagedFlow": true, | ||
| "flow.enabled": true, | ||
| "eslint.autoFixOnSave": true, | ||
| "javascript.validate.enable": false, |
There was a problem hiding this comment.
Enabling this to true breaks hell loose, with TSC trying to check .js files ignoring tsconfig.json. But once migrated, we would want this enabled by default.
src/components/button/__tests__/__snapshots__/Button.test.tsx.snap
Outdated
Show resolved
Hide resolved
src/components/loading-indicator/__tests__/LoadingIndicatorWrapper.test.tsx
Show resolved
Hide resolved
priyajeet
force-pushed
the
typescript
branch
3 times, most recently
from
5e62132 to
a0616aa
Compare
| @@ -18,16 +18,44 @@ module.exports = { | |||
| 'react/no-access-state-in-setstate': 'off', // fixme | |||
| 'react/no-array-index-key': 'off', // fixme | |||
| 'react/no-this-in-sfc': 'off', | |||
| 'import/no-unresolved': 'off', // fixme | |||
There was a problem hiding this comment.
This is to keep eslint happy from when a JS file tries to import a TS module
.eslintrc.js
Outdated
| { | ||
| files: ['*.stories.tsx', '*.test.tsx'], | ||
| rules: { | ||
| 'import/no-extraneous-dependencies': 'off' |
There was a problem hiding this comment.
we dont want to consider stories as something needed at runtime. So imports dont need to be in deps section and can remain in dev deps.
src/components/loading-indicator/__tests__/__snapshots__/LoadingIndicatorWrapper.test.tsx.snap
Show resolved
Hide resolved
priyajeet
force-pushed
the
typescript
branch
2 times, most recently
from
d86e1b3 to
a125dd7
Compare
.eslintrc.js
Outdated
| files: ['*.ts', '*.tsx'], | ||
| rules: { | ||
| '@typescript-eslint/explicit-function-return-type': 'off', // fixme | ||
| 'import/extensions': 'off', |
There was a problem hiding this comment.
This rule forces extension on imports ==> import Button from '../button.js'
priyajeet
force-pushed
the
typescript
branch
from
a4d8ba9 to
d8e5419
Compare
| const titleID = `${this.id}-title`; | ||
|
|
||
| // Make sure parent doesn't accidentally override these values | ||
| const svgProps: Record<string, string | number | React.ReactNode> = omit(rest, ['role', 'aria-labelledby']); |
There was a problem hiding this comment.
Dealing with rest and omit is tricky.
There was a problem hiding this comment.
where does the Record type come from?
There was a problem hiding this comment.
Its native to TypeScript
| @@ -0,0 +1,41 @@ | |||
| // @flow | |||
There was a problem hiding this comment.
I don't think these .flow files were checked in before, they were generated and put next to the ES module dist exports for the npm package
There was a problem hiding this comment.
These flow files are to keep VSCode happy with flow+ts together while you are working within BUIE. Its similar to the ones we generated at publish time but those were only for consuming projects. These ones are for BUIE itself now. These will all be temporary and will be nuked eventually after migration.
| await writeFile(outputJs, `${moduleHeader}${moduleString}`); | ||
| await writeFile(outputJs, `${moduleHeader}${moduleString}`); // deprecate eventually | ||
| await writeFile(outputTs, `${moduleTSHeader}${moduleString}`); | ||
| execSync(`yarn eslint --fix ${outputTs}`); |
There was a problem hiding this comment.
Only linting ts for now. Helps not needing eslint disable directives.
| return <div className="avatar-image" />; | ||
| } | ||
|
|
||
| jest.mock('../AvatarImage', () => MockAvatarImage); |
There was a problem hiding this comment.
I think this was here so it didn't make an actual network call for the invalid images
| @@ -0,0 +1,83 @@ | |||
| /* File auto-generated */ | |||
There was a problem hiding this comment.
Is this the strictest possible constant definition in TS? In flow you can also do this to make it a constant string instead of regular string:
const black: '#000' = '#000'
There was a problem hiding this comment.
Good question, I can make it generate that. Ideally (we can do this eventually) this should be an enum like this:
enum bdlVars {
WHITE = '#fff',
BLACK = '#000',
...
}And then accessed via bdlVars.WHITE
priyajeet
force-pushed
the
typescript
branch
from
d8e5419 to
2ed6c8a
Compare
| @@ -5,7 +5,6 @@ module.exports = { | |||
| rules: { | |||
| camelcase: 'off', | |||
| 'class-methods-use-this': 'off', | |||
| 'formatjs/enforce-description': 'error', // fixme | |||
There was a problem hiding this comment.
moved to @box/frontend
| }, | ||
| plugins: ['formatjs'], |
There was a problem hiding this comment.
moved to @box/frontend
priyajeet
changed the title
| @@ -1,4 +1,6 @@ | |||
| const path = require('path'); | |||
| // const typescriptDocGen = require('react-docgen-typescript'); | |||
There was a problem hiding this comment.
Do we need these commented lines? Same for the exports below.
There was a problem hiding this comment.
They are currently left there for reference/documentation. Will be nuked eventually. Just dont want to go hunting the web again if that needs to be introduced.
so that parent projects get tricked
BREAKING CHANGE: Peer deps have been updated and added.
tsconfig.json
Outdated
| }, | ||
| "extends": "@box/frontend/ts/tsconfig", |
There was a problem hiding this comment.
If paths came directly from the base config, that will be relative to that config/project and not this current inheriting project. tsconfig is missing tokens like <rootDir> that can be used in the path to avoid ambiguity. See microsoft/TypeScript#25430
tsconfig.json
Outdated
| "files": ["./.storybook/typings.d.ts"] | ||
| "exclude": ["src/**/*.js", "node_modules", "es", "dist"], | ||
| "files": [".storybook/typings.d.ts"], | ||
| "include": ["src/**/*"] |
There was a problem hiding this comment.
Rather than using both exclude and include, could we do "include": ["src/**/*.ts", "src/**/*.tsx"]?
relative to this current project. See microsoft/TypeScript#25430
This PR introduces typescript into BUIE. It includes:
Parent projects doing local symlink-ing with BUIE will need to update their configs to allow TS.