-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[TM] Add spec for ExceptionsManager #24900
Conversation
/** | ||
* Available only on Android | ||
*/ | ||
+dismissRedbox: () => void; |
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.
what do we do about it @rickhanlonii?
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.
Should we need to split it to Android and iOS? if not, seems we need to label it for Codegen.
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.
For additional properties we have decided to generate it on both platforms and no-op on one, so for this you can update the comment to // Android only
and keep it in one file
Splitting out would be required for props/events that conflict
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.
Also, we should make platform-specific properties optional. We don't want this to be a required method on the protocol we generate for iOS.
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.
LGTM, thanks! @thymikee
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.
Looks good! However, we need to make a few changes before we can land this.
/** | ||
* Available only on Android | ||
*/ | ||
+dismissRedbox: () => void; |
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.
Also, we should make platform-specific properties optional. We don't want this to be a required method on the protocol we generate for iOS.
|
||
import type {TurboModule} from 'RCTExport'; | ||
import * as TurboModuleRegistry from 'TurboModuleRegistry'; | ||
import type {StackFrame} from './Devtools/parseErrorStack'; |
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.
Unfortunately, our current TM codegen doesn't support type imports. I wonder if we should move StackFrame
to this Spec file and instead import the type in parseErrorStack
. If not, then we should just declare these twice.
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.
Moved the type def to NativeExceptionsManager as there wasn't too many call-sites to change
bf1e285
to
c39de6b
Compare
exceptionId: number, | ||
) => void; | ||
// Android only | ||
+dismissRedbox?: () => void; |
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.
@RSNara 🤔 Seems sentIntent
of IntentAndroid
not marks optional
https://github.com/facebook/react-native/pull/24877/files#diff-7033e846872152e52446abb20da3efb8R25, should we also changed that?
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.
Yeah, nice catch. We should change that.
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.
Actually, no. I was wrong earlier about our codegen output. Turns out, regardless of if a method is optional or not, it'll generate a required method in native. So, making this optional doesn't do anything.
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.
👌 Get it.
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.
Looks good! Thanks for addressing the comments. 👍
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.
@RSNara has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This pull request was successfully merged by @thymikee in 8ea749a. When will my fix make it into a release? | Upcoming Releases |
Summary: Part of facebook#24875, adds a spec for ExceptionsManager ## Changelog [General] [Added] - TM Add spec for ExceptionsManager Pull Request resolved: facebook#24900 Reviewed By: fkgozali Differential Revision: D15434006 Pulled By: RSNara fbshipit-source-id: 1a505744a84c0c4ac3a9fac6c91a391fbd8a9f46
Summary
Part of #24875, adds a spec for ExceptionsManager
Changelog
[General] [Added] - TM Add spec for ExceptionsManager
Test Plan
Flow passes.