-
Notifications
You must be signed in to change notification settings - Fork 46
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
chore: set locale and timezone using the react-native-localize library #373
Conversation
Size Change: +283 B (+0.26%) Total Size: 110 kB
ℹ️ View Unchanged
|
@k11kirky tagged you because |
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.
PR Summary
This PR replaces Expo's localization with react-native-localize for handling locale and timezone settings in the PostHog React Native SDK, addressing compatibility issues when Expo is not being used.
- Added
react-native-localize
as a new peer dependency in/posthog-react-native/package.json
- Implemented new
OptionalReactNativeLocalize.ts
for handling locale/timezone when Expo is unavailable - Modified
/posthog-react-native/src/native-deps.tsx
to usereact-native-localize
as fallback for non-Expo apps - Breaking change: Removed several deprecated device info methods (is24Hour, getTimezone, etc.) from react-native-device-info
- Added timezone/locale functionality using
getLocales()
andgetTimeZone()
from react-native-localize
💡 (5/5) You can turn off certain types of comments like style here!
7 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
OptionalReactNativeLocalize = require('react-native-localize') | ||
} catch (e) {} |
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.
style: Empty catch block could silently fail. Consider logging the error when in debug mode.
@@ -33,6 +33,7 @@ | |||
"react-native": "^0.69.1", | |||
"react-native-device-info": "^10.3.0", | |||
"react-native-navigation": "^6.0.0", | |||
"react-native-localize": "^3.0.0", |
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.
logic: react-native-localize is added as a devDependency but not as a peerDependency. Since it's replacing core functionality, it should probably be added to peerDependencies with an appropriate version range.
const localesFn = OptionalReactNativeLocalize.getLocales | ||
if (localesFn) { | ||
const locales = localesFn() | ||
|
||
if (locales && locales.length > 0) { | ||
const languageTag = locales[0].languageTag | ||
if (languageTag) { | ||
properties.$locale = languageTag | ||
} | ||
} | ||
} |
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.
style: Consider adding error handling for localesFn() call which could throw an exception
const timezoneFn = OptionalReactNativeLocalize.getTimeZone | ||
if (timezoneFn) { | ||
const timezone = timezoneFn() | ||
|
||
if (timezone) { | ||
properties.$timezone = timezone | ||
} | ||
} |
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.
style: Consider adding error handling for timezoneFn() call which could throw an exception
@@ -72,7 +72,7 @@ export class WrappedMessages extends AnthropicOriginal.Messages { | |||
return parentPromise.then((value) => { | |||
const passThroughStream = new PassThrough({ objectMode: true }) | |||
let accumulatedContent = '' | |||
let usage: { inputTokens: number; outputTokens: number } = { | |||
const usage: { inputTokens: number; outputTokens: number } = { |
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.
this should have stayed let - its gets reasigned under
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.
the linter did it automatically btw (eslint, prettier), but as I can see it, the usage props are resigned, but not the usage
itself.
Happy to change it back but am I missing something?
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.
the 2 openai usage
gets reasigned, but not the anthropic one.
Problem
Closes #286
Changes
Release info Sub-libraries affected
Bump level
Libraries affected
Changelog notes