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: Adds support for Jest snapshot testing #375

Merged

Conversation

fjtrujy
Copy link
Contributor

@fjtrujy fjtrujy commented Mar 26, 2018

Description

This PR just makes the library compatible when you are running native snapshots using jest.

Compatibility

OS Implemented
iOS
Android
Windows
Web

Checklist

  • I have tested this on a device/simulator for each compatible OS

@ammichael
Copy link

Why this isn't merged yet?

@fjtrujy
Copy link
Contributor Author

fjtrujy commented Nov 30, 2018

Guys, could this PR be merged?

Thanks

@mikehardy
Copy link
Collaborator

Probably hasn't been merged because there was no explanation of why the change was necessary. Not everyone understands everything (like Jest snapshots) and/or what everything's requirements are and how that fits into the current codebase. So explaining things is important.

Just from reading it I have inferred (possibly correctly) that for Jest snapshots to work it can't rely on native code (so something like the cutout / empty polyfill for react native web is necessary) but that web things aren't available either (so the one web thing we have - the user agent - also doesn't work). Thus the proposed implementation is to make a completely empty polyfill for Jest snapshots, then layer the web polyfill on top of that. Is that correct?

If so, the PR could have said so, like "Jest snapshot testing can't use native items or web items, so needs a completely empty polyfill, this PR alters the existing web polyfill to be completely empty then re-implements web as a layer on the empty one, so Jest snapshots and web polyfill are both supported".

Assuming that's all true, confirm it's still necessary, patch up the conflict, test it's working with Jest and Web (there's a sample repo linked on #323 I think) and I can merge

@mikehardy
Copy link
Collaborator

@fjtrujy did you see my last comment? tagging as you may not have seen the notification

@fjtrujy
Copy link
Contributor Author

fjtrujy commented Mar 28, 2019

Hello @mikehardy,
First of all sorry for the delay, but I skip your previous message.
If I remember properly (it was already ONE YEAR AGO....) that was to improve the websupport and make it more generic.

I suffered 2 issues in the past:

  1. AppVersion function was not available for web (I don't know if right now it is), so in order to have a common behavior, I export the same functions for web and native.
  2. In order to make your module compatible with Snapshot Testing (https://jestjs.io/docs/en/snapshot-testing), it required in the past some improvements.

I think that this is not about a polyfill, is just to expose the same functions in all the platforms that we support (make sense).

Let me try to rebase and see if my changes are still needed and valuable.

@fjtrujy
Copy link
Contributor Author

fjtrujy commented Mar 28, 2019

I have rebased the branch & updated the PR.

Thanks!

@mikehardy
Copy link
Collaborator

Thanks! And yes, it is one year and a day now 😴 - but I just started on the project and I'm clearing the queue. Better late than never I hope

@mikehardy
Copy link
Collaborator

I just pulled the branch and checked it locally in an iOS emulator and it seems fine for the main / native pathway. There were no steps to reproduce or example how to set this up for jest snapshots to show what was expected / what broke so I can't verify if it fixes that pathway but this seems okay to merge

@mikehardy mikehardy merged commit f14e352 into react-native-device-info:master Mar 28, 2019
@fjtrujy
Copy link
Contributor Author

fjtrujy commented Mar 28, 2019

Cool, I have an additional change in my fork that I'm going to PR as well.

Thanks

@mikehardy
Copy link
Collaborator

It's out!

https://www.npmjs.com/package/react-native-device-info/v/1.2.0

I'd love anything else you have, happy to help merge things in

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

Successfully merging this pull request may close these issues.

None yet

3 participants