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

Add new HOC for instrumenting components manually #266

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

salockhart
Copy link

Description

What does this PR add/fix/change?

Our project uses Pickers quite extensively, from https://github.com/react-native-picker/picker.

However, using these pickers has one limitation - unlike Touchables, etc. they are not instrumented with Heap for autocapture.

This means that we need to try and do it ourselves, either with custom events or another method. This is less than ideal, and is prone to errors.

This PR instead adds a new HOC that can be used to wrap the picker and wire it up with autocapture, like so:

import Heap from '@heap/react-native-heap';
import { Picker } from '@react-native-picker/picker';
const InstrumentedPicker = Heap.withHeapAutocapture(Picker, 'onFocus');

Previously, I had proposed a hard-coded solution (#265), but I found that almost immediately lacking. This instead covers far more cases.

Once it has been instrumented, whenever the picker opens (onFocus) we get a touch event auto captured in Heap. This then allows us to define our events directly in Heap with ease!


While working on this, we also had need to instrument some native components that, while not Touchables exactly, have onPress as a prop and behave in a similar way. To get this, we took a similar approach of using the HOC. This PR adds those HOCs to index.d.ts to ensure they can be used.

Is there anything reviewers should pay special attention to?

I cloned a lot of the work in withHeapFocusableAutocapture from prior work - please let me know if it is correct.
I also tried my best to define the HOCs in TypeScript, but I may have missed something there too.

Are there any breaking changes?

No, these are only additions.

Test Plan

How were these changes tested?

I applied these changes to my local project with patch-package and verified that:

  • the instrumentation on the picker worked correctly
  • all types passed type checking

Were additional test cases added?

No, I did not see any test cases for the other HOCs, so I have not here. Please let me know what to cover and I will give it a try.

Was there manual testing?

Yes, I tested manually in my project.

Checklist

  • Detox tests pass (only Heap employees are able run these)
  • If this is a bugfix/feature, the changelog has been updated

@bnickel
Copy link
Collaborator

bnickel commented Nov 11, 2021

Hi, just commenting to let you know that I see this PR and it looks promising. There's currently some internal blockage on accepting PRs, but when that clears I'll look at this.

@lukaszKowal-datasite
Copy link

Hi, just commenting to let you know that I see this PR and it looks promising. There's currently some internal blockage on accepting PRs, but when that clears I'll look at this.

Hi @bnickel I know it has been a while, but is the internal blockage resolved?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants