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

detect react-native #137

Merged
merged 2 commits into from
Apr 29, 2020
Merged

detect react-native #137

merged 2 commits into from
Apr 29, 2020

Conversation

pedrouid
Copy link
Contributor

I noticed that #8 was open for almost 5 years so I thought I would contribute a PR myself

@pedrouid
Copy link
Contributor Author

This PR could be further improved by expecting a 'react-native' as peer-dependency conditionally and check for Platform.OS to verify whether it's iOS and Android. However I feel the added complexity is unnecessary for this package and this simple check would be enough

@pedrouid
Copy link
Contributor Author

Something I haven't considered is whether document is also undefined for bots but assuming they won't match the navigator.product check, I don't think this should conflict

@DamonOehlman
Copy link
Owner

@pedrouid Thanks heaps mate! That issue has been open for a long time and really has needed someone who actively works with react-native to take it on, so thanks for doing that. I'll review the PR in the next day and get it into master.

@DamonOehlman DamonOehlman self-requested a review April 28, 2020 00:14
@DamonOehlman DamonOehlman merged commit 50e40bb into DamonOehlman:master Apr 29, 2020
@DamonOehlman
Copy link
Owner

@pedrouid Thanks again for doing this work. As you can see I've merged the changes into master. I was wondering if prior to me pushing a version update, you would be able to take it for a spin against a react-native app using the dependency of DamonOehlman/detect-browser. If that all works, then I'll push an update.

@pedrouid
Copy link
Contributor Author

pedrouid commented May 1, 2020

Hey @DamonOehlman, where is it published? Could you publish it with a different tag like rc or beta or next?

@DamonOehlman
Copy link
Owner

@pedrouid Good question - I was thinking of back to when I did "old school" pure JS implementations and you could just throw a reference to a github project in the package.json and it would resolve to the github master branch. That still works of course, but it doesn't work with a typescript project....

Reading, and then re-reading the logic I think it's safe to publish a new minor version so I'm going to do that now and I'll give you a heads up when it's live.

@DamonOehlman
Copy link
Owner

Done - 5.1.0

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

Successfully merging this pull request may close these issues.

2 participants