expo / sentry-expo

MIT License
203 stars 83 forks source link

feat: upgrade `@sentry/react-native` #301

Closed SimenB closed 1 year ago

SimenB commented 1 year ago

Checklist

Why

The current version is quite outdated.

Closes #293

How

Upgraded all @sentry packages, and made the necessary type change.

I'm not sure if this should be considered a breaking change?

Test Plan

Not sure - hopefully you have some test app you can use? 😀

kbrandwijk commented 1 year ago

Would this be breaking because Sentry now expects these fields to be either set or undefined, and expo-device returns null?

SimenB commented 1 year ago

I don't think so - I assume the type is stricter than the implementation 🙂 Haven't checked tho.

The reason I think this might be breaking is simply because the user's version must match

kbrandwijk commented 1 year ago

Thank you very much for your contribution. I've wrapped these changes into #302, as those are also breaking changes and I'd rather ship a single major release.

SimenB commented 1 year ago

you can land this on a separate branch (then land that in bulk for release) so the commit (and the contribution) is kept in history. I personally don't care (just happy to see movement! 🥳), but I think in general that's a better approach than bundling it all under "eas support"

kbrandwijk commented 1 year ago

You're right, I forget that matters for external contributions.

SimenB commented 1 year ago

Thanks!

kbrandwijk commented 1 year ago

@SimenB If you restore the other 2 branches, I'd be happy to do the same there.

SimenB commented 1 year ago

Done 👍 I'm happy to rebase if needed (I assume they conflict with this PR)

(rebased them)

kbrandwijk commented 1 year ago

It doesn't let me reopen the PRs image Could I bother you to put up 2 new PRs? You can just reference the old ones for details. Sorry for the inconvenience.

SimenB commented 1 year ago

hah, should have held off on the rebase 😛