[devx] Fix error reporting for module errors #34650
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@rickhanlonii has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Base commit: 3e97d5f |
Base commit: 3e97d5f |
lunaleaps
pushed a commit
to lunaleaps/react-native
that referenced
this pull request
Sep 12, 2022
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
facebook-github-bot
pushed a commit
that referenced
this pull request
Sep 13, 2022
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
|
👍 |
|
This pull request was successfully merged by @rickhanlonii in af0e6cd. When will my fix make it into a release? | Upcoming Releases |
facebook-github-bot
pushed a commit
that referenced
this pull request
Oct 27, 2022
Summary: In 2017, React published v15.5 which extracted the built-in `prop types` to a separate package to reflect the fact that not everybody uses them. In 2018, React Native started to remove `PropTypes` from React Native for the same reason. In 0.68 React Native introduced a deprecation warning which notified users that the change was coming, and in 0.69 we removed the PropTypes entirely. The feedback we've received from the community is that there has not been enough time to migrate libraries off of PropTypes. This has resulted in users needing to patch the React Native package `index.js` file directly to add back the PropTypes, instead of migrating off of them. We can empathize with this fix short term (it unblocks the upgrade) but long term this patch will cause users to miss important changes to `index.js`, and add a maintenance cost for users. Part of the reason there was not enough time is that we didn't do a good job surfacing libraries that were using PropTypes. This means, when you got a deprecation warning, it wasn't clear where the source of the usage was (either in your code or in a library). So even if you wanted to migrate, it was difficult to know where to actually make the change. In the next release, we've made it easier to find call sites using deprecated types by [fixing the code frame in errors](react-native-community/cli#1699) reporting in LogBox, and ensuring that [the app doesn't crash without a warning](#34650). This should make it easier to identify exactly where the deprecated usage is, so you can migrate it. To help users get off of the patch, and allow more time to migrate, we're walking back the removal of PropTypes, and keeping it as a deprecation for a couple more versions. We ask that you either migrate off PropTypes to a type system like TypeScript, or migrate to the `deprecated-react-native-prop-types` package. Once we feel more confident that the community has migrated and will not need to patch React Native in order to fix this issue, we'll remove the PropTypes again. **If you have any trouble finding the source of the PropType usage, please file an issue so we can help track it down with you.** Changelog: [General][Changed] - Add back deprecated PropTypes Reviewed By: yungsters Differential Revision: D40725705 fbshipit-source-id: 8ce61be30343827efd6dc89a012eeef0b6676deb
OlimpiaZurek
pushed a commit
to OlimpiaZurek/react-native
that referenced
this pull request
May 22, 2023
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
OlimpiaZurek
pushed a commit
to OlimpiaZurek/react-native
that referenced
this pull request
May 22, 2023
Summary: Addresses some of facebook#34649 ## 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 --> [General] [Fixed] - Error reporting for module errors Pull Request resolved: facebook#34650 Test Plan: ### Before <img width="1728" alt="Screen Shot 2022-09-08 at 10 01 17 AM" src="https://user-images.githubusercontent.com/2440089/189425932-783b857d-59d3-4979-aecc-3de9d5bc6b0a.png"> ### After <img width="1728" alt="Screen Shot 2022-09-09 at 2 35 13 PM" src="https://user-images.githubusercontent.com/2440089/189425885-112130f4-aebd-4d84-b6c4-19b4dbb907e8.png"> Reviewed By: mdvacca Differential Revision: D39395369 Pulled By: rickhanlonii fbshipit-source-id: f60882e0fefbd3eb8481d251556afe5cda60c034
OlimpiaZurek
pushed a commit
to OlimpiaZurek/react-native
that referenced
this pull request
May 22, 2023
Summary: In 2017, React published v15.5 which extracted the built-in `prop types` to a separate package to reflect the fact that not everybody uses them. In 2018, React Native started to remove `PropTypes` from React Native for the same reason. In 0.68 React Native introduced a deprecation warning which notified users that the change was coming, and in 0.69 we removed the PropTypes entirely. The feedback we've received from the community is that there has not been enough time to migrate libraries off of PropTypes. This has resulted in users needing to patch the React Native package `index.js` file directly to add back the PropTypes, instead of migrating off of them. We can empathize with this fix short term (it unblocks the upgrade) but long term this patch will cause users to miss important changes to `index.js`, and add a maintenance cost for users. Part of the reason there was not enough time is that we didn't do a good job surfacing libraries that were using PropTypes. This means, when you got a deprecation warning, it wasn't clear where the source of the usage was (either in your code or in a library). So even if you wanted to migrate, it was difficult to know where to actually make the change. In the next release, we've made it easier to find call sites using deprecated types by [fixing the code frame in errors](react-native-community/cli#1699) reporting in LogBox, and ensuring that [the app doesn't crash without a warning](facebook#34650). This should make it easier to identify exactly where the deprecated usage is, so you can migrate it. To help users get off of the patch, and allow more time to migrate, we're walking back the removal of PropTypes, and keeping it as a deprecation for a couple more versions. We ask that you either migrate off PropTypes to a type system like TypeScript, or migrate to the `deprecated-react-native-prop-types` package. Once we feel more confident that the community has migrated and will not need to patch React Native in order to fix this issue, we'll remove the PropTypes again. **If you have any trouble finding the source of the PropType usage, please file an issue so we can help track it down with you.** Changelog: [General][Changed] - Add back deprecated PropTypes Reviewed By: yungsters Differential Revision: D40725705 fbshipit-source-id: 8ce61be30343827efd6dc89a012eeef0b6676deb
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Addresses some of #34649
Changelog
[General] [Fixed] - Error reporting for module errors
Test Plan
Before
After