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.

Sign up for GitHub

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

Jump to bottom

Discussion: Refactor background observers to use the same eventEmitter #233

Open
CaptainJeff opened this issue Jul 12, 2022 · 5 comments · May be fixed by #312
Open

Discussion: Refactor background observers to use the same eventEmitter #233

CaptainJeff opened this issue Jul 12, 2022 · 5 comments · May be fixed by #312
Labels
enhancement New feature or request

Comments

@CaptainJeff
Copy link

CaptainJeff commented Jul 12, 2022 •
edited

Is your feature request related to a problem? Please describe.
In order to listen to multiple measurement types in the background, you have to create 4 event emitters for every type. e.g.


NativeAppEventEmitter.addListener('healthKit:HeartRate:setup:success', callback)
NativeAppEventEmitter.addListener('healthKit:HeartRate:new', callback)
NativeAppEventEmitter.addListener('healthKit:HeartRate:setup:failure', callback)
NativeAppEventEmitter.addListener('healthKit:HeartRate:failure', callback)

NativeAppEventEmitter.addListener('healthKit:Workout:setup:success', callback)
NativeAppEventEmitter.addListener('healthKit:Workout:new', callback)
NativeAppEventEmitter.addListener('healthKit:Workout:setup:failure', callback)
NativeAppEventEmitter.addListener('healthKit:Workout:failure', callback)

Describe the solution you'd like
It seems like we could refactor this to reuse the 4 events and pass the measurement type as the parameter instead of an empty object

Describe alternatives you've considered
None

Additional context
We could use the following five events (the four listed above plus "sample)

        @"healthKit:new",
        @"healthKit:failure",
        @"healthKit:enabled",
        @"healthKit:sample",
        @"healthKit:setup:success",
        @"healthKit:setup:failure"

And instead of inserting the type into the string of the event emitter

NSString *successEvent = [NSString stringWithFormat:@"healthKit:%@:setup:success", type];

We could instead pass

NSString *successEvent = [NSString stringWithFormat:@"healthKit:setup:success"];
[self sendEventWithName:successEvent body:type];

And in the js side

eventListener = eventEmitter.addListener(`healthKit:new`, (type) => {
   // Inspect the measurement type (workout, heartrate, etc...)
})

I can open a PR for this. I just want to make sure the community agreed this was the right approach first
@CaptainJeff CaptainJeff added the enhancement New feature or request label Jul 12, 2022
@CaptainJeff
Copy link
Author

@GGGava Hey man. Just wanted to loop back to this and see if you had any thoughts on this approach.

@GGGava
Copy link
Collaborator

GGGava commented Jan 17, 2023

Hey @CaptainJeff, thank you for your contribution!

This seems like a very good idea, I just have some questions:

1- Why do we need this new event sample? What is the difference between sample and new?
2- Is it possible to implement this without breaking our current method? We would need to deprecate the current method but keep it working so we don't force ppl to update to this new method without previous warning.

@CaptainJeff
Copy link
Author

  1. sample is actually an event we currently have implemented

    react-native-health/RCTAppleHealthKit/RCTAppleHealthKit.m

    Line 656 in 1a5557f

    NSArray *templates = @[@"healthKit:%@:new", @"healthKit:%@:failure", @"healthKit:%@:enabled", @"healthKit:%@:sample", @"healthKit:%@:setup:success", @"healthKit:%@:setup:failure"];

react-native-health/RCTAppleHealthKit/RCTAppleHealthKit+Queries.m

Line 1049 in 1a5557f

NSString *successEvent = [NSString stringWithFormat:@"healthKit:%@:sample", type];

I don't really use it but I think it's used for different measurement categories
2) I think I could get it to work to be backwards compatible with some stricter checks in various places if necessary. Might be a little funky and verbose but could get it

@GGGava
Copy link
Collaborator

GGGava commented Jan 17, 2023

Cool, thank you very much, I believe we can proceed with that 🚀

Regarding the sample event, it's not mentioned in the docs, could you also check if it's not dead code? 😅

@CaptainJeff CaptainJeff linked a pull request May 18, 2023 that will close this issue
Open
6 tasks
@CaptainJeff
Copy link
Author

@GGGava I opened a PR for this #312 and made it backwards compatible. Let me know if you have any questions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: refactor background observers and event listeners coachcare/react-native-health-1
2 participants
@CaptainJeff @GGGava