-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Move TypeScript declarations into react-native #34614
Conversation
LogError: Error: Failed to load parser '@typescript-eslint/parser' declared in '../../.eslintrc.js » @react-native-community/eslint-config#overrides[1]': Cannot find module 'typescript'
Require stack:
- /home/runner/work/react-native/react-native/node_modules/@typescript-eslint/typescript-estree/dist/parser.js
- /home/runner/work/react-native/react-native/node_modules/@typescript-eslint/typescript-estree/dist/index.js
- /home/runner/work/react-native/react-native/node_modules/@typescript-eslint/parser/dist/parser.js
- /home/runner/work/react-native/react-native/node_modules/@typescript-eslint/parser/dist/index.js
- /home/runner/work/react-native/react-native/node_modules/@eslint/eslintrc/dist/eslintrc.cjs
at Function.Module._resolveFilename (node:internal/modules/cjs/loader:956:15)
at Function.Module._load (node:internal/modules/cjs/loader:804:27)
at Function._module2.default._load (/home/runner/work/react-native/react-native/node_modules/override-require/dist/overrideRequire.js:43:25)
at Module.require (node:internal/modules/cjs/loader:1028:19)
at require (node:internal/modules/cjs/helpers:102:18)
at Object.<anonymous> (/home/runner/work/react-native/react-native/node_modules/@typescript-eslint/typescript-estree/dist/parser.js:35:25)
at Module._compile (node:internal/modules/cjs/loader:1126:14)
at Object.customModuleHandler (/home/runner/work/react-native/react-native/node_modules/danger/distribution/runner/runners/inline.js:129:28)
at Module.load (node:internal/modules/cjs/loader:1004:32)
at Function.Module._load (node:internal/modules/cjs/loader:839:12) {
code: 'MODULE_NOT_FOUND',
requireStack: [
'/home/runner/work/react-native/react-native/node_modules/@typescript-eslint/typescript-estree/dist/parser.js',
'/home/runner/work/react-native/react-native/node_modules/@typescript-eslint/typescript-estree/dist/index.js',
'/home/runner/work/react-native/react-native/node_modules/@typescript-eslint/parser/dist/parser.js',
'/home/runner/work/react-native/react-native/node_modules/@typescript-eslint/parser/dist/index.js',
'/home/runner/work/react-native/react-native/node_modules/@eslint/eslintrc/dist/eslintrc.cjs'
]
}
danger-results://tmp/danger-results.json |
Base commit: d15a82d |
Base commit: d15a82d |
.eslintignore
Outdated
@@ -9,3 +9,4 @@ Libraries/vendor/**/* | |||
node_modules/ | |||
packages/*/node_modules | |||
packages/react-native-codegen/lib | |||
types/**/* |
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.
I'm not sure if I should be doing this.
7361226
to
885e8a3
Compare
Currently trying to figure out why Danger eslint fails. |
.eslintrc.js
Outdated
parser: '@typescript-eslint/parser', | ||
plugins: ['@typescript-eslint/eslint-plugin'], | ||
rules: { | ||
'@typescript-eslint/no-unused-vars': 0, |
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.
nit: ESLint maps 0, 1, 2 to "off", "warn", "error". So using "off" instead of 0 might be more readable.
@@ -23,7 +23,8 @@ | |||
"eslint-plugin-prettier": "^4.2.1", | |||
"eslint-plugin-react": "^7.30.1", | |||
"eslint-plugin-react-hooks": "^4.6.0", | |||
"eslint-plugin-react-native": "^4.0.0" | |||
"eslint-plugin-react-native": "^4.0.0", | |||
"typescript": "^4.8.2" |
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.
This package is used externally as well, but the .eslintrc
which adds TS usage is internal to the repo. It would be ideal if the dependency is only added in the internal config.
Otherwise if used in the community ESLint config, it should maybe be a peerDependency with a loose constraint like eslint/prettier, so the app can control the version.
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.
Yes, I originally had it under repo-config/package.json
but have this as currently trying to debug why danger isn't working. This didn't end up helping so will update this!
types/tslint.json
Outdated
@@ -0,0 +1,23 @@ | |||
{ | |||
"extends": "@definitelytyped/dtslint/dtslint.json", | |||
"rules": { |
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.
In case you haven't stumbled onto it yet, a lot of these have direct mappings to ESLint rules documented at https://typescript-eslint.io/rules/.
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.
Oh nice! Thank you! I had not found that! I can get rid of this then!
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.
Hmm so I think a lot of the tslint rules we have here are custom for TS declarations and don't have a eslint equivalent. In that case, we'd still need to run dtslint
and extend @definitelytyped/dtslint/dtslint.json
(which defines some of the rules we're disabling here). So I think any additions to eslint would be un-necessary
f7c71f2
to
bd026bb
Compare
Added fixes to our danger setup to #34652. Once that lands, and we rebase this, then danger should stop failing. |
Summary: When working on facebook#34614, danger is failing because it doesn't share `node_modules` with the root directory where `typescript` is installed as we added it as a parser in our eslint config. By setting `bots` as a yarn workspace, dependencies are all installed under the root `node_modules` folder and in local testing (detailed in test section) we no longer have the `typescript module not found` error. However, danger will continue to fail on facebook#34614 as the `danger_pr` Github action runs from what's defined on `main`. Once these changes land, I can rebase facebook#34614 on it and danger's eslint should pass. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [Internal][Fixed] - Add `bots` directory as a yarn workspace and update `danger_pr` Github action Pull Request resolved: facebook#34652 Test Plan: To verify this fix I had to run: ``` react-native $ yarn && cd bots react-native/bots$ yarn run danger pr facebook#34614 ``` which resulted in ``` ❯ yarn run danger pr facebook#34614 yarn run v1.22.19 $ lunaleaps/react-native/node_modules/.bin/danger pr facebook#34614 Starting Danger PR on facebook#34614 Danger: ✓ found only warnings, not failing the build ## Warnings :lock: package.json - <i>Changes were made to package.json. This will require a manual import by a Facebook employee.</i> ✨ Done in 12.78s. ``` Verified this also on another PR: ``` yarn run danger pr facebook#34650 ``` Reviewed By: NickGerleman Differential Revision: D39435286 Pulled By: lunaleaps fbshipit-source-id: b560d92635be8d95baf27274f745315aaf56d748
Summary: allow-large-files When working on #34614, danger is failing because it doesn't share `node_modules` with the root directory where `typescript` is installed as we added it as a parser in our eslint config. By setting `bots` as a yarn workspace, dependencies are all installed under the root `node_modules` folder and in local testing (detailed in test section) we no longer have the `typescript module not found` error. However, danger will continue to fail on #34614 as the `danger_pr` Github action runs from what's defined on `main`. Once these changes land, I can rebase #34614 on it and danger's eslint should pass. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [Internal][Fixed] - Add `bots` directory as a yarn workspace and update `danger_pr` Github action Pull Request resolved: #34652 Test Plan: To verify this fix I had to run: ``` react-native $ yarn && cd bots react-native/bots$ yarn run danger pr #34614 ``` which resulted in ``` ❯ yarn run danger pr #34614 yarn run v1.22.19 $ lunaleaps/react-native/node_modules/.bin/danger pr #34614 Starting Danger PR on #34614 Danger: ✓ found only warnings, not failing the build ## Warnings :lock: package.json - <i>Changes were made to package.json. This will require a manual import by a Facebook employee.</i> ✨ Done in 12.78s. ``` Verified this also on another PR: ``` yarn run danger pr #34650 ``` Reviewed By: NickGerleman Differential Revision: D39435286 Pulled By: lunaleaps fbshipit-source-id: 8c82f49facf162f4fc0918e3abd95eb7e4ad1e37
3b92183
to
5720242
Compare
5720242
to
19ca785
Compare
@lunaleaps has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This pull request was exported from Phabricator. Differential Revision: D39479137 |
Summary: ## Changelog [General] [Added] - Add `types` folder to house TypeScript types. Release TypesScript types with react-native and eventually deprecate [types/react-native](https://www.npmjs.com/package/types/react-native). The current plan is to release types/react-native for 0.70 and 0.71 while also maintaining types here. This will result in some double maintenance until 0.72 but will give community time to move off of types/react-native. After this lands, there have been changes on `main` of types that we need to update. Then, when we release 0.71 from DefinitelyTyped, we can simply copy over the `types` folder from this repo. Pull Request resolved: facebook#34614 Test Plan: `yarn run test-typescript` for linting types * Created a new project using the TS template and my local clone of `react-native` on this branch. `npx react-native init MyTSApp --version <path-to-my-local-rn-repo> --template react-native-template-typescript` * Updated the `package.json` to remove `types/react-native` * Deleted my node_modules and re-ran yarn * Opened MyTSApp in VSCode and verified the type suggestions appeared and cmd+click to defnitions took me to the node_module dependency `react-native/types` ## Danger is failing on this PR and it's expected as it runs off the changes on `main`. [This is expected](https://docs.github.com/en/github-ae@latest/actions/using-workflows/events-that-trigger-workflows?fbclid=IwAR2_AE0Jwndt8Gu-iTQnxGxLJq7nakbi7sz8jwZ6U62JWLSdcZuvjcQ6WvE#pull_request_target). However testing it locally passes ``` $ react-native/packages/react-native-bots ❯ yarn danger pr facebook#34614 yarn run v1.22.19 $ ..react-native/node_modules/.bin/danger pr facebook#34614 Starting Danger PR on facebook#34614 Danger: ✓ found only warnings, not failing the build ## Warnings :lock: package.json - <i>Changes were made to package.json. This will require a manual import by a Facebook employee.</i> ✨ Done in 13.24s. ``` Differential Revision: D39479137 Pulled By: lunaleaps fbshipit-source-id: 3e2202b0436e7cb75101b042be916d3ba211d9f5
19ca785
to
3f7439d
Compare
@@ -93,7 +94,8 @@ | |||
"test-android-instrumentation": "yarn run docker-build-android && yarn run test-android-run-instrumentation", | |||
"test-android-unit": "yarn run docker-build-android && yarn run test-android-run-unit", | |||
"test-android-e2e": "yarn run docker-build-android && yarn run test-android-run-e2e", | |||
"test-ios": "./scripts/objc-test.sh test" | |||
"test-ios": "./scripts/objc-test.sh test", | |||
"test-typescript": "dtslint types" |
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.
Is anything running these tests during build/pr?
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.
Not yet, will address this!
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.
I've added running dtslint as part of the analyze_code
CircleCI tests
This pull request was exported from Phabricator. Differential Revision: D39479137 |
Summary: ## Changelog [General] [Added] - Add `types` folder to house TypeScript types. Release TypesScript types with react-native and eventually deprecate [types/react-native](https://www.npmjs.com/package/types/react-native). The current plan is to release types/react-native for 0.70 and 0.71 while also maintaining types here. This will result in some double maintenance until 0.72 but will give community time to move off of types/react-native. After this lands, there have been changes on `main` of types that we need to update. Then, when we release 0.71 from DefinitelyTyped, we can simply copy over the `types` folder from this repo. Pull Request resolved: facebook#34614 Test Plan: `yarn run test-typescript` for linting types * Created a new project using the TS template and my local clone of `react-native` on this branch. `npx react-native init MyTSApp --version <path-to-my-local-rn-repo> --template react-native-template-typescript` * Updated the `package.json` to remove `types/react-native` * Deleted my node_modules and re-ran yarn * Opened MyTSApp in VSCode and verified the type suggestions appeared and cmd+click to defnitions took me to the node_module dependency `react-native/types` ## Danger is failing on this PR and it's expected as it runs off the changes on `main`. [This is expected](https://docs.github.com/en/github-ae@latest/actions/using-workflows/events-that-trigger-workflows?fbclid=IwAR2_AE0Jwndt8Gu-iTQnxGxLJq7nakbi7sz8jwZ6U62JWLSdcZuvjcQ6WvE#pull_request_target). However testing it locally passes ``` $ react-native/packages/react-native-bots ❯ yarn danger pr facebook#34614 yarn run v1.22.19 $ ..react-native/node_modules/.bin/danger pr facebook#34614 Starting Danger PR on facebook#34614 Danger: ✓ found only warnings, not failing the build ## Warnings :lock: package.json - <i>Changes were made to package.json. This will require a manual import by a Facebook employee.</i> ✨ Done in 13.24s. ``` Differential Revision: D39479137 Pulled By: lunaleaps fbshipit-source-id: 2883124712c756f731fe1bbb45e3629480296d82
3f7439d
to
d25a676
Compare
Summary: ## Changelog [General] [Added] - Add `types` folder to house TypeScript types. Release TypesScript types with react-native and eventually deprecate [types/react-native](https://www.npmjs.com/package/types/react-native). The current plan is to release types/react-native for 0.70 and 0.71 while also maintaining types here. This will result in some double maintenance until 0.72 but will give community time to move off of types/react-native. After this lands, there have been changes on `main` of types that we need to update. Then, when we release 0.71 from DefinitelyTyped, we can simply copy over the `types` folder from this repo. Pull Request resolved: facebook#34614 Test Plan: `yarn run test-typescript` for linting types * Created a new project using the TS template and my local clone of `react-native` on this branch. `npx react-native init MyTSApp --version <path-to-my-local-rn-repo> --template react-native-template-typescript` * Updated the `package.json` to remove `types/react-native` * Deleted my node_modules and re-ran yarn * Opened MyTSApp in VSCode and verified the type suggestions appeared and cmd+click to defnitions took me to the node_module dependency `react-native/types` ## Danger is failing on this PR and it's expected as it runs off the changes on `main`. [This is expected](https://docs.github.com/en/github-ae@latest/actions/using-workflows/events-that-trigger-workflows?fbclid=IwAR2_AE0Jwndt8Gu-iTQnxGxLJq7nakbi7sz8jwZ6U62JWLSdcZuvjcQ6WvE#pull_request_target). However testing it locally passes. Once merged, and these changes are on `main`, danger will pass again. ``` $ react-native/packages/react-native-bots ❯ yarn danger pr facebook#34614 yarn run v1.22.19 $ ..react-native/node_modules/.bin/danger pr facebook#34614 Starting Danger PR on facebook#34614 Danger: ✓ found only warnings, not failing the build ## Warnings :lock: package.json - <i>Changes were made to package.json. This will require a manual import by a Facebook employee.</i> ✨ Done in 13.24s. ``` Reviewed By: mdvacca Differential Revision: D39479137 Pulled By: lunaleaps fbshipit-source-id: a7398492386abee9543b3bb5d98682cdf15f54da
This pull request was exported from Phabricator. Differential Revision: D39479137 |
d25a676
to
fbcba62
Compare
This pull request was successfully merged by @lunaleaps in 6b2a511. When will my fix make it into a release? | Upcoming Releases |
Summary: From changes in #34614, I forgot to actually export the types directory. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [Internal] - Add types directory as part of the npm package Pull Request resolved: #34727 Test Plan: Verified on [`build_npm_package_1`](https://app.circleci.com/pipelines/github/facebook/react-native/15799/workflows/4fd57bfc-0142-49d9-a1a6-83fa43f0e371/jobs/295438) job where we create the commitly, that `types` folder is included and `__typetests__` are ignored Reviewed By: cipolleschi Differential Revision: D39646310 Pulled By: lunaleaps fbshipit-source-id: 361f7925fa1cdc30d0865d291933c7fce9e0fa49
@@ -93,7 +94,8 @@ | |||
"test-android-instrumentation": "yarn run docker-build-android && yarn run test-android-run-instrumentation", | |||
"test-android-unit": "yarn run docker-build-android && yarn run test-android-run-unit", | |||
"test-android-e2e": "yarn run docker-build-android && yarn run test-android-run-e2e", | |||
"test-ios": "./scripts/objc-test.sh test" | |||
"test-ios": "./scripts/objc-test.sh test", | |||
"test-typescript": "dtslint types" |
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.
I don't know if you explored this, but there is https://github.com/jest-community/jest-runner-tsd allowing you to run type tests with Jest instead of adding a new tool.
You might have ported tests from DT, in which case using the same tool is easier, I guess 😀
Also, super excited you've done this! 🎉
Summary: allow-large-files When working on facebook#34614, danger is failing because it doesn't share `node_modules` with the root directory where `typescript` is installed as we added it as a parser in our eslint config. By setting `bots` as a yarn workspace, dependencies are all installed under the root `node_modules` folder and in local testing (detailed in test section) we no longer have the `typescript module not found` error. However, danger will continue to fail on facebook#34614 as the `danger_pr` Github action runs from what's defined on `main`. Once these changes land, I can rebase facebook#34614 on it and danger's eslint should pass. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [Internal][Fixed] - Add `bots` directory as a yarn workspace and update `danger_pr` Github action Pull Request resolved: facebook#34652 Test Plan: To verify this fix I had to run: ``` react-native $ yarn && cd bots react-native/bots$ yarn run danger pr facebook#34614 ``` which resulted in ``` ❯ yarn run danger pr facebook#34614 yarn run v1.22.19 $ lunaleaps/react-native/node_modules/.bin/danger pr facebook#34614 Starting Danger PR on facebook#34614 Danger: ✓ found only warnings, not failing the build ## Warnings :lock: package.json - <i>Changes were made to package.json. This will require a manual import by a Facebook employee.</i> ✨ Done in 12.78s. ``` Verified this also on another PR: ``` yarn run danger pr facebook#34650 ``` Reviewed By: NickGerleman Differential Revision: D39435286 Pulled By: lunaleaps fbshipit-source-id: 8c82f49facf162f4fc0918e3abd95eb7e4ad1e37
Summary: ## Changelog [General] [Added] - Add `types` folder to house TypeScript types. Release TypesScript types with react-native and eventually deprecate [types/react-native](https://www.npmjs.com/package/types/react-native). The current plan is to release types/react-native for 0.70 and 0.71 while also maintaining types here. This will result in some double maintenance until 0.72 but will give community time to move off of types/react-native. After this lands, there have been changes on `main` of types that we need to update. Then, when we release 0.71 from DefinitelyTyped, we can simply copy over the `types` folder from this repo. Pull Request resolved: facebook#34614 Test Plan: `yarn run test-typescript` for linting types * Created a new project using the TS template and my local clone of `react-native` on this branch. `npx react-native init MyTSApp --version <path-to-my-local-rn-repo> --template react-native-template-typescript` * Updated the `package.json` to remove `types/react-native` * Deleted my node_modules and re-ran yarn * Opened MyTSApp in VSCode and verified the type suggestions appeared and cmd+click to defnitions took me to the node_module dependency `react-native/types` ## Danger is failing on this PR and it's expected as it runs off the changes on `main`. [This is expected](https://docs.github.com/en/github-ae@latest/actions/using-workflows/events-that-trigger-workflows?fbclid=IwAR2_AE0Jwndt8Gu-iTQnxGxLJq7nakbi7sz8jwZ6U62JWLSdcZuvjcQ6WvE#pull_request_target). However testing it locally passes. Once merged, and these changes are on `main`, danger will pass again. ``` $ react-native/packages/react-native-bots ❯ yarn danger pr facebook#34614 yarn run v1.22.19 $ ..react-native/node_modules/.bin/danger pr facebook#34614 Starting Danger PR on facebook#34614 Danger: ✓ found only warnings, not failing the build ## Warnings :lock: package.json - <i>Changes were made to package.json. This will require a manual import by a Facebook employee.</i> ✨ Done in 13.24s. ``` Reviewed By: mdvacca Differential Revision: D39479137 Pulled By: lunaleaps fbshipit-source-id: 1d506f812d566b783b6e79104cd6339b077a42a7
Summary: From changes in facebook#34614, I forgot to actually export the types directory. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [Internal] - Add types directory as part of the npm package Pull Request resolved: facebook#34727 Test Plan: Verified on [`build_npm_package_1`](https://app.circleci.com/pipelines/github/facebook/react-native/15799/workflows/4fd57bfc-0142-49d9-a1a6-83fa43f0e371/jobs/295438) job where we create the commitly, that `types` folder is included and `__typetests__` are ignored Reviewed By: cipolleschi Differential Revision: D39646310 Pulled By: lunaleaps fbshipit-source-id: 361f7925fa1cdc30d0865d291933c7fce9e0fa49
Summary
Release TypesScript types with react-native and eventually deprecate @types/react-native.
The current plan is to release @types/react-native for 0.70 and 0.71 while also maintaining types here. This will result in some double maintenance until 0.72 but will give community time to move off of @types/react-native.
After this lands, there have been changes on
main
of types that we need to update. Then, when we release 0.71 from DefinitelyTyped, we can simply copy over thetypes
folder from this repo.Changelog
[General] [Added] - Add
types
folder to house TypeScript types.Test Plan
yarn run test-typescript
for linting typesreact-native
on this branch.npx react-native init MyTSApp --version <path-to-my-local-rn-repo> --template react-native-template-typescript
package.json
to remove@types/react-native
react-native/types
Danger is failing on this PR and it's expected
as it runs off the changes on
main
. This is expected. However testing it locally passes