-
Notifications
You must be signed in to change notification settings - Fork 932
chore: migrate tools to TypeScript
#296
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
Changes from all commits
d977006
d94eda8
068fc64
adb9126
998bb75
c6f31ee
7484280
775bdfe
31595fd
6cdfd83
b5a11b4
985043a
5a4dfe1
b4884ea
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 |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { | ||
| // See http://go.microsoft.com/fwlink/?LinkId=827846 | ||
| // for the documentation about the extensions.json format | ||
| "recommendations": [ | ||
| "dbaeumer.vscode-eslint", | ||
| "redhat.vscode-yaml", | ||
| "flowtype.flow-for-vscode", | ||
| "esbenp.prettier-vscode" | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| { | ||
| "editor.rulers": [80], | ||
| "files.exclude": { | ||
| "**/.git": true, | ||
| "**/node_modules": true, | ||
| "**/build": true | ||
| }, | ||
| "editor.formatOnSave": true, | ||
| "flow.useNPMPackagedFlow": true, | ||
| "javascript.validate.enable": false, | ||
| "prettier.eslintIntegration": true, | ||
| "eslint.validate": [ | ||
| "javascript", | ||
| "javascriptreact", | ||
| { | ||
| "language": "typescript", | ||
| "autoFix": true | ||
| }, | ||
| { | ||
| "language": "typescriptreact", | ||
|
Member
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. Should we commit
Member
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. I've added it on purpose because of eslint integration issues that we fixed by adding these entries
Member
Author
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. We should. It gives contributors recommendations for plugins, and provides better integration with vscode eslint plugin. We have to configure additional rule for plugin to check typescript files. |
||
| "autoFix": true | ||
| } | ||
| ] | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,14 +4,16 @@ | |
| "packages/*" | ||
| ], | ||
| "scripts": { | ||
| "prebuild": "yarn build:ts", | ||
| "build": "node ./scripts/build.js", | ||
| "build-clean": "rm -rf ./packages/*/build", | ||
| "build:ts": "node ./scripts/buildTs.js", | ||
| "build-clean": "rm -rf ./packages/*/build ./packages/*/tsconfig.tsbuildinfo", | ||
| "watch": "node ./scripts/watch.js", | ||
| "test": "jest", | ||
| "test:ci:unit": "jest packages --ci --coverage", | ||
| "test:ci:e2e": "jest e2e --ci -i", | ||
| "lint": "eslint --ext .js,.ts . --cache --report-unused-disable-directives", | ||
|
Member
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. Do we need to specify extensions manually even if we use ESLint config?
Member
Author
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. For Typescript, yes. |
||
| "test:ci:cocoapods": "ruby packages/platform-ios/native_modules.rb", | ||
| "lint": "eslint . --cache --report-unused-disable-directives", | ||
| "flow-check": "flow check", | ||
| "postinstall": "yarn build", | ||
| "publish": "yarn build-clean && yarn build && lerna publish" | ||
|
|
@@ -22,12 +24,16 @@ | |
| "@babel/plugin-transform-strict-mode": "^7.0.0", | ||
| "@babel/preset-env": "^7.0.0", | ||
| "@babel/preset-flow": "^7.0.0", | ||
| "@react-native-community/eslint-config": "^0.0.3", | ||
| "@babel/preset-typescript": "^7.3.3", | ||
| "@react-native-community/eslint-config": "^0.0.5", | ||
| "@types/jest": "^24.0.11", | ||
| "@types/node": "^11.13.0", | ||
| "babel-jest": "^24.6.0", | ||
| "babel-plugin-module-resolver": "^3.2.0", | ||
| "chalk": "^2.4.2", | ||
| "eslint": "^5.10.0", | ||
| "eslint-import-resolver-alias": "^1.1.2", | ||
| "eslint-import-resolver-typescript": "^1.1.1", | ||
| "eslint-plugin-import": "^2.17.0", | ||
| "execa": "^1.0.0", | ||
| "flow-bin": "^0.97.0", | ||
|
|
@@ -38,6 +44,7 @@ | |
| "metro-memory-fs": "^0.53.1", | ||
| "micromatch": "^3.1.10", | ||
| "mkdirp": "^0.5.1", | ||
| "string-length": "^2.0.0" | ||
| "string-length": "^2.0.0", | ||
| "typescript": "^3.4.5" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| const getDefaultUserTerminal = (): ?string => | ||
| const getDefaultUserTerminal = (): string | undefined => | ||
| process.env.REACT_TERMINAL || process.env.TERM_PROGRAM; | ||
|
|
||
| export default getDefaultUserTerminal; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,22 +4,11 @@ | |
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| * | ||
| * @flow | ||
| */ | ||
|
|
||
| import {groupBy} from 'lodash'; | ||
| import mime from 'mime'; | ||
|
|
||
| /** | ||
| * Since there are no officially registered MIME types | ||
| * for ttf/otf yet http://www.iana.org/assignments/media-types/media-types.xhtml, | ||
| * we define two non-standard ones for the sake of parsing | ||
| */ | ||
| mime.define({ | ||
| 'font/opentype': ['otf'], | ||
| 'font/truetype': ['ttf'], | ||
thymikee marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }); | ||
|
|
||
| /** | ||
| * Given an array of files, it groups it by it's type. | ||
| * Type of the file is inferred from it's mimetype based on the extension | ||
|
|
@@ -32,5 +21,5 @@ mime.define({ | |
| * the returned object will be: {font: ['fonts/a.ttf'], image: ['images/b.jpg']} | ||
| */ | ||
| export default function groupFilesByType(assets: Array<string>) { | ||
| return groupBy(assets, type => mime.lookup(type).split('/')[0]); | ||
| return groupBy(assets, type => (mime.getType(type) || '').split('/')[0]); | ||
|
Member
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. When this can return
Member
Author
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. When you try ot get mime type which doesn't exist.
Member
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. Was it always like that, even with the previous version? Trying to get an idea whether we had a hidden
Member
Author
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. Probably yes, because |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,18 +16,17 @@ import fetch from 'node-fetch'; | |
| * - `not_running`: the packager nor any process is running on the expected port. | ||
| * - `unrecognized`: one other process is running on the port we expect the packager to be running. | ||
| */ | ||
| function isPackagerRunning( | ||
| async function isPackagerRunning( | ||
| packagerPort: string = process.env.RCT_METRO_PORT || '8081', | ||
| ): Promise<'running' | 'not_running' | 'unrecognized'> { | ||
| return fetch(`http://localhost:${packagerPort}/status`).then( | ||
| res => | ||
| res | ||
| .text() | ||
| .then(body => | ||
| body === 'packager-status:running' ? 'running' : 'unrecognized', | ||
| ), | ||
| () => 'not_running', | ||
| ); | ||
| try { | ||
| const result = await fetch(`http://localhost:${packagerPort}/status`); | ||
| const body = await result.text(); | ||
|
|
||
| return body === 'packager-status:running' ? 'running' : 'unrecognized'; | ||
| } catch (_error) { | ||
| return 'not_running'; | ||
| } | ||
|
Member
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. Feels like a good opportunity for enum here
Member
Author
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. Yes, but the point of this PR was to configure Typescript ecosystem and prepare for migration, not refactoring every LOC.
Member
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, the PR says "migrate", not "prepare for migration", so that's the reason I am asking. I'd love us to migrate to Typescript fully and embrace its features where possible. Shall we open a follow-up issue to track the backlog of things like this, and possibly others that you may already have in mind?
Member
Author
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. Ye, sure. |
||
| } | ||
|
|
||
| export default isPackagerRunning; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,3 @@ | ||
| /** | ||
| * @flow | ||
| */ | ||
| import chalk from 'chalk'; | ||
|
|
||
| const SEPARATOR = ', '; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "extends": "../../tsconfig.json", | ||
| "compilerOptions": { | ||
| "rootDir": "src", | ||
| "outDir": "build" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| /** | ||
| * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| */ | ||
|
|
||
| 'use strict'; | ||
|
|
||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
|
|
||
| const chalk = require('chalk'); | ||
| const execa = require('execa'); | ||
| const {getPackages, adjustToTerminalWidth, OK} = require('./helpers'); | ||
|
|
||
| const packages = getPackages(); | ||
|
|
||
| const packagesWithTs = packages.filter(p => | ||
| fs.existsSync(path.resolve(p, 'tsconfig.json')), | ||
| ); | ||
|
|
||
| const args = [ | ||
| path.resolve( | ||
| require.resolve('typescript/package.json'), | ||
| '..', | ||
| require('typescript/package.json').bin.tsc, | ||
| ), | ||
| '-b', | ||
| ...packagesWithTs, | ||
| ...process.argv.slice(2), | ||
| ]; | ||
|
|
||
| console.log(chalk.inverse('Building TypeScript definition files')); | ||
| process.stdout.write(adjustToTerminalWidth('Building\n')); | ||
|
|
||
| try { | ||
| execa.sync('node', args, {stdio: 'inherit'}); | ||
| process.stdout.write(`${OK}\n`); | ||
| } catch (e) { | ||
| process.stdout.write('\n'); | ||
| console.error( | ||
| chalk.inverse.red('Unable to build TypeScript definition files'), | ||
| ); | ||
| console.error(e.stack); | ||
| process.exitCode = 1; | ||
| } |
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.
we should add TS plugins for ESLint, similar to: https://github.com/callstack/eslint-config-callstack#typescript
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.
TS plugins are included in react-native eslint configuration. https://github.com/facebook/react-native/blob/master/packages/eslint-config-react-native-community/index.js#L48