[TM] Add spec for HeapCapture#24899
Conversation
facebook-github-bot
added
p: Callstack
react-native-bot
added
the
Type: Enhancement
ericlewis
left a comment
ericlewis
left a comment
There was a problem hiding this comment.
mostly looks good, unsure about the require on batched bridge though.
| ); | ||
| BatchedBridge.registerLazyCallableModule('HeapCapture', () => | ||
| require('../Utilities/HeapCapture'), | ||
| require('../HeapCapture/HeapCapture'), |
There was a problem hiding this comment.
requiring like this means you should also grab default i think.
There was a problem hiding this comment.
i'm not sure if there should be default since export looks like this:
module.exports = HeapCapture;
There was a problem hiding this comment.
Ah gotcha, thought this was coming direct from NativeHeapCapture
ericlewis
left a comment
ericlewis
left a comment
There was a problem hiding this comment.
looks great, just a few things!
| +removeListeners: (eventName: string, handler: Function) => void; | ||
| } | ||
|
|
||
| export default TurboModuleRegistry.getEnforcing<Spec>('HeapCapture'); |
There was a problem hiding this comment.
this is android only, so use platform to conditionally export
| +captureComplete: (path: string, error: ?string) => void; | ||
|
|
||
| // Events | ||
| +addListener: (eventName: string, handler: Function) => Object; |
There was a problem hiding this comment.
is HeapCapture an emitter? If not, remove these
There was a problem hiding this comment.
yes, sorry there is no event emitter in java class.
|
@ericlewis i fixed comments you left, could you review it once again? |
ericlewis
left a comment
ericlewis
left a comment
There was a problem hiding this comment.
This looks pretty good to me! I would suggest perhaps putting it back in utilities however.
RSNara
left a comment
RSNara
left a comment
There was a problem hiding this comment.
Thanks for the PR!
This looks pretty good!
| // Common interface | ||
| +captureHeap: (path: string) => void; | ||
|
|
||
| // Android only |
There was a problem hiding this comment.
nit: It looks like this module is only available on Android. So, this comment is confusing. Should it be here?
| import {Platform} from 'react-native'; | ||
|
|
||
| export interface Spec extends TurboModule { | ||
| // Common interface |
There was a problem hiding this comment.
nit: Should this be here? (see previous comment).
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@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 @wojteg1337 in 94029ee. When will my fix make it into a release? | Upcoming Releases |
Summary: Part of facebook#24875, adds a spec for HeapCapture ## Changelog [General] [Added] - TM Spec for HeapCapture Pull Request resolved: facebook#24899 Reviewed By: fkgozali Differential Revision: D15393464 Pulled By: RSNara fbshipit-source-id: d8778285753ce8dbc87204ecfbddfa7339acd264
6 participants
Summary
Part of #24875, adds a spec for HeapCapture
Changelog
[General] [Added] - TM Spec for HeapCapture
Test Plan
Flow passes