Do not suppress IllegalArgumentExceptions during TurboModule creation #41509
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
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D51395165 |
Base commit: fa89dd6 |
javache
added a commit
to javache/react-native
that referenced
this pull request
Nov 16, 2023
…facebook#41509) Summary: We currently ignore `IllegalArgumentException` being thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor. TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios. Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException. Differential Revision: D51395165
javache
force-pushed
the
export-D51395165
branch
from
16b09d2 to
139445c
Compare
|
This pull request was exported from Phabricator. Differential Revision: D51395165 |
javache
added a commit
to javache/react-native
that referenced
this pull request
Nov 16, 2023
…facebook#41509) Summary: We currently ignore `IllegalArgumentException` being thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor. TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios. Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException. Reviewed By: RSNara Differential Revision: D51395165
javache
force-pushed
the
export-D51395165
branch
from
139445c to
d47f8fa
Compare
|
This pull request was exported from Phabricator. Differential Revision: D51395165 |
javache
added a commit
to javache/react-native
that referenced
this pull request
Nov 16, 2023
…facebook#41509) Summary: We currently ignore `IllegalArgumentException` being thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor. TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios. Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException. Reviewed By: RSNara Differential Revision: D51395165
javache
force-pushed
the
export-D51395165
branch
from
d47f8fa to
505090a
Compare
|
This pull request was exported from Phabricator. Differential Revision: D51395165 |
javache
added a commit
to javache/react-native
that referenced
this pull request
Nov 16, 2023
…facebook#41509) Summary: We currently ignore `IllegalArgumentException` being thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor. TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios. Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException. Reviewed By: RSNara Differential Revision: D51395165
javache
force-pushed
the
export-D51395165
branch
from
505090a to
8808c29
Compare
|
This pull request was exported from Phabricator. Differential Revision: D51395165 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D51395165 |
javache
force-pushed
the
export-D51395165
branch
from
8808c29 to
f83f1b4
Compare
javache
added a commit
to javache/react-native
that referenced
this pull request
Nov 20, 2023
…facebook#41509) Summary: We currently ignore `IllegalArgumentException` being thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor. TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios. Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException. Reviewed By: RSNara Differential Revision: D51395165
javache
added a commit
to javache/react-native
that referenced
this pull request
Nov 20, 2023
…facebook#41509) Summary: We currently ignore `IllegalArgumentException` being thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor. TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios. Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException. Reviewed By: RSNara Differential Revision: D51395165
javache
force-pushed
the
export-D51395165
branch
from
f83f1b4 to
cef0114
Compare
|
This pull request was exported from Phabricator. Differential Revision: D51395165 |
javache
added a commit
to javache/react-native
that referenced
this pull request
Nov 20, 2023
…facebook#41509) Summary: We currently ignore `IllegalArgumentException` being thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor. TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios. Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException. Reviewed By: RSNara Differential Revision: D51395165
javache
force-pushed
the
export-D51395165
branch
from
cef0114 to
8537d0f
Compare
|
This pull request was exported from Phabricator. Differential Revision: D51395165 |
javache
added a commit
to javache/react-native
that referenced
this pull request
Nov 20, 2023
…facebook#41509) Summary: We currently ignore `IllegalArgumentException` being thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor. TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios. Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException. Reviewed By: RSNara Differential Revision: D51395165
javache
force-pushed
the
export-D51395165
branch
from
8537d0f to
e69defd
Compare
|
This pull request was exported from Phabricator. Differential Revision: D51395165 |
javache
force-pushed
the
export-D51395165
branch
from
e69defd to
68575bc
Compare
|
This pull request was exported from Phabricator. Differential Revision: D51395165 |
javache
added a commit
to javache/react-native
that referenced
this pull request
Nov 20, 2023
…facebook#41509) Summary: We currently ignore `IllegalArgumentException` being thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor. TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios. Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException. Reviewed By: RSNara Differential Revision: D51395165
javache
force-pushed
the
export-D51395165
branch
from
68575bc to
e8caca1
Compare
|
This pull request was exported from Phabricator. Differential Revision: D51395165 |
javache
added a commit
to javache/react-native
that referenced
this pull request
Nov 20, 2023
…facebook#41509) Summary: We currently ignore `IllegalArgumentException` being thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor. TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios. Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException. Reviewed By: RSNara Differential Revision: D51395165
javache
force-pushed
the
export-D51395165
branch
from
e8caca1 to
eee8721
Compare
|
This pull request was exported from Phabricator. Differential Revision: D51395165 |
javache
added a commit
to javache/react-native
that referenced
this pull request
Nov 20, 2023
…facebook#41509) Summary: We currently ignore `IllegalArgumentException` being thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor. TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios. Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException. Reviewed By: RSNara Differential Revision: D51395165
javache
force-pushed
the
export-D51395165
branch
from
eee8721 to
1fdf459
Compare
|
This pull request was exported from Phabricator. Differential Revision: D51395165 |
javache
added a commit
to javache/react-native
that referenced
this pull request
Nov 20, 2023
…facebook#41509) Summary: We currently ignore `IllegalArgumentException` being thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor. TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios. Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException. Reviewed By: RSNara Differential Revision: D51395165
javache
force-pushed
the
export-D51395165
branch
from
1fdf459 to
e8fce17
Compare
|
This pull request was exported from Phabricator. Differential Revision: D51395165 |
javache
added a commit
to javache/react-native
that referenced
this pull request
Nov 20, 2023
…facebook#41509) Summary: We currently ignore `IllegalArgumentException` being thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor. TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios. Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException. Reviewed By: RSNara Differential Revision: D51395165
javache
force-pushed
the
export-D51395165
branch
from
e8fce17 to
9bb057c
Compare
…facebook#41509) Summary: We currently ignore `IllegalArgumentException` being thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor. TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios. Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException. Reviewed By: RSNara Differential Revision: D51395165
|
This pull request was exported from Phabricator. Differential Revision: D51395165 |
javache
force-pushed
the
export-D51395165
branch
from
9bb057c to
1dd0e5b
Compare
|
This pull request was exported from Phabricator. Differential Revision: D51395165 |
|
This pull request has been merged in 228193d. |
Othinn
pushed a commit
to Othinn/react-native
that referenced
this pull request
Jan 9, 2024
…facebook#41509) Summary: Pull Request resolved: facebook#41509 We currently ignore `IllegalArgumentException` being thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor. TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios. Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException. Reviewed By: RSNara Differential Revision: D51395165 fbshipit-source-id: 1eea1db6c7e3313a36d24e7837b36a3d0fccc718
javache
added a commit
to javache/react-native
that referenced
this pull request
Feb 22, 2024
…gerDelegate Summary: Now that 0.74 has been cut, we can drop this warning and let the exception bubble up. In facebook#41509 we stopped masking this. Changelog: [Android][Changed] Throwing IllegalArgumentException from ReactPackage is no longer suppressed Reviewed By: cortinico, cipolleschi Differential Revision: D54068429
javache
added a commit
to javache/react-native
that referenced
this pull request
Feb 22, 2024
…gerDelegate (facebook#43152) Summary: Now that 0.74 has been cut, we can drop this warning and let the exception bubble up. In facebook#41509 we stopped masking this. Changelog: [Android][Changed] Throwing IllegalArgumentException from ReactPackage is no longer suppressed Reviewed By: cortinico, cipolleschi Differential Revision: D54068429
facebook-github-bot
pushed a commit
that referenced
this pull request
Feb 22, 2024
…gerDelegate (#43152) Summary: Pull Request resolved: #43152 Now that 0.74 has been cut, we can drop this warning and let the exception bubble up. In #41509 we stopped masking this. Changelog: [Android][Changed] Throwing IllegalArgumentException from ReactPackage is no longer suppressed Reviewed By: cortinico, cipolleschi Differential Revision: D54068429 fbshipit-source-id: c2f780ccfefabf2334c94b632bca93242af86008
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:
We currently ignore
IllegalArgumentExceptionbeing thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor.TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios.
Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException.
Differential Revision: D51395165