Skip to content
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

[devx] Fix error reporting for module errors #34650

Closed
wants to merge 1 commit into from

Conversation

rickhanlonii
Copy link
Member

Summary

Addresses some of #34649

Changelog

[General] [Fixed] - Error reporting for module errors

Test Plan

Before

Screen Shot 2022-09-08 at 10 01 17 AM

After

Screen Shot 2022-09-09 at 2 35 13 PM

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Sep 9, 2022
@facebook-github-bot
Copy link
Contributor

@rickhanlonii has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,638,381 -4,610
android hermes armeabi-v7a 7,050,452 -4,657
android hermes x86 7,939,997 -4,542
android hermes x86_64 7,912,116 -4,526
android jsc arm64-v8a 9,512,747 -3,222
android jsc armeabi-v7a 8,288,263 -3,309
android jsc x86 9,452,131 -3,171
android jsc x86_64 10,043,279 -3,091

Base commit: 3e97d5f
Branch: main

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 3e97d5f
Branch: main

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
@MarcoDeJong
Copy link

👍

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @rickhanlonii in af0e6cd.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Oct 19, 2022
@rickhanlonii rickhanlonii deleted the rh-fix-errors branch October 26, 2022 18:00
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
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants