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

Change React Native Version import to the one from Platform #3949

Merged
merged 8 commits into from
Aug 1, 2024

Conversation

svbutko
Copy link
Contributor

@svbutko svbutko commented Jul 15, 2024

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

The import for selection of React Native version, both for type and value happens via JS constant, while there's an documented way to get it from Platform: https://reactnative.dev/docs/platform#constants

It's also available in 0.65 which makes it work for all versions:
https://reactnative-archive-august-2023.netlify.app/docs/0.65/platform#constants

💡 Motivation and Context

While working with other repos than react-native like react-native-tvos or react-native-web with certain TS configs, there are issues like this one:

node_modules/@sentry/react-native/dist/js/utils/rnlibrariesinterface.d.ts:1:37 - error TS7016: Could not find a declaration file for module 'react-native/Libraries/Core/ReactNativeVersion'. '/Users/**/zattoo/aurora/node_modules/react-native/Libraries/Core/ReactNativeVersion.js' implicitly has an 'any' type.
  If the 'react-native-tvos' package actually exposes this module, try adding a new declaration (.d.ts) file containing `declare module 'react-native/Libraries/Core/ReactNativeVersion';`

1 import type { version as RNV } from 'react-native/Libraries/Core/ReactNativeVersion';

This PR solves this import issue

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • All tests passing
  • No breaking changes

🔮 Next steps

@krystofwoldrich
Copy link
Member

Hi @svbutko,
thank you for the fix. Excuse the long waiting time, I was OoO.

It definitely make sense to use the public documented export instead of the internal lib.

Could you check why yarn test fails after the change. I've already added changelog, but we need the tests to succeed to merge the change.

@svbutko
Copy link
Contributor Author

svbutko commented Jul 31, 2024

Hi @krystofwoldrich
No problem at all!

Checked the tests, and added a fix for potential missing constants (e.g. testing or going beyond supported platforms)

Sometimes there's a failing stalltracking.background.test.ts test: Stall tracking should set _isBackground to false, update _lastIntervalMs, and call _iteration when state is active and _timeout is not null but it's not related to this PR and it's floating

Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @svbutko!

It looks good now. 🚀

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