-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat: Adds support for Jest snapshot testing #375
Conversation
Why this isn't merged yet? |
7480518
to
ee58ab4
Compare
Guys, could this PR be merged? Thanks |
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 |
@fjtrujy did you see my last comment? tagging as you may not have seen the notification |
Hello @mikehardy, I suffered 2 issues in the past:
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. |
ee58ab4
to
ea5f49a
Compare
I have rebased the branch & updated the PR. Thanks! |
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 |
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 |
Cool, I have an additional change in my fork that I'm going to PR as well. Thanks |
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 |
Description
This PR just makes the library compatible when you are running native snapshots using jest.
Compatibility
Checklist