-
-
Notifications
You must be signed in to change notification settings - Fork 535
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
fix(isNodeProcess): avoid undefined error #255
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for spotting this, @jameslahm! I've had one suggestion to the changes, could you please let me know what you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome!
I'd love to see a unit test in a React Native environment, but to reliably bootstrap one would be an overkill in our testing pipeline. We'd have to maintain that navigator
manually.
Let's wait for the CI to pass.
Yes, there should be a unit test. I add a React Native Environment test just now. Copying environment settings from |
Hey. I've rebased the branch against the latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great changes. Thank you, @jameslahm, and welcome to contributors!
The changes look good! Thanks! |
I just found a problem, that is because we mock |
Hi, Thanks for the amazing tool. I try to use it in a browser, but
global is not defined
came out. I think it maybe caused byisNodeProcess
function. Change it totypeof process ==='object'
may solve this problem.