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

feat: new expo plugin #110

Merged
merged 3 commits into from
Jun 30, 2024
Merged

feat: new expo plugin #110

merged 3 commits into from
Jun 30, 2024

Conversation

matinzd
Copy link
Owner

@matinzd matinzd commented Jun 29, 2024

Copy link
Collaborator

@TheRogue76 TheRogue76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is perfectly fine, but i do have some worries about the plugin, specifically https://github.com/matinzd/expo-health-connect/blob/9d30d099577a9ffa66598b9099399a9074967099/src/withHealthConnect.ts#L35 and https://github.com/matinzd/expo-health-connect/blob/9d30d099577a9ffa66598b9099399a9074967099/src/withHealthConnect.ts#L51
Realistically, if you check that the activity is not null, it wouldn't make any sense for it NOT to have at least one, but i would like for it to double check anyway and maybe throw an error, not just return (cause then there is no happy path and the installation has gone horribly wrong).

@matinzd
Copy link
Owner Author

matinzd commented Jun 30, 2024

Yeah I totally agree with you. I was just making it work for now and didn't even publish it on npm. Created a PR to address those:

matinzd/expo-health-connect#1

@matinzd matinzd merged commit ebb5fd4 into main Jun 30, 2024
3 checks passed
@matinzd matinzd deleted the new-expo-plugin branch June 30, 2024 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment