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

chore: ref to allow sync storage friendlier #198

Merged
merged 15 commits into from
Mar 4, 2024
Merged

Conversation

marandaneto
Copy link
Member

@marandaneto marandaneto commented Mar 1, 2024

Problem

Tempt to fix #96

Changes

Release info Sub-libraries affected

Bump level

  • Major
  • Minor
  • Patch

Libraries affected

  • All of them
  • posthog-web
  • posthog-node
  • posthog-react-native

Changelog notes

  • Added support for X

Copy link

github-actions bot commented Mar 1, 2024

Size Change: 0 B 🆕

Total Size: 0 B

compressed-size-action

@marandaneto marandaneto requested a review from benjackwhite March 1, 2024 11:31
@marandaneto
Copy link
Member Author

Definitely breaking changes but in good time if the approach is better for sync storages.
cc @filipef101 WDYT?

}
this.setupBootstrap(options)

// It is possible that the old library was used so we try to get the legacy distinctID
if (!this._semiAsyncStorage?.getItem(PostHogPersistedProperty.AnonymousId)) {
if (!this._storage?.getItem(PostHogPersistedProperty.AnonymousId)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically none of this applies if the storage isn't async - the only reason we do things the way we do with async storage is to have allowed people to move from the old native SDKs.

posthog-react-native/src/storage.ts Show resolved Hide resolved
posthog-react-native/src/posthog-rn.ts Outdated Show resolved Hide resolved
posthog-react-native/src/posthog-rn.ts Outdated Show resolved Hide resolved
@benjackwhite benjackwhite marked this pull request as ready for review March 1, 2024 15:08
Copy link

@filipef101 filipef101 left a comment

Choose a reason for hiding this comment

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

I'm happy with this, and think fits my needs. Might be nitpicking or confused, the whole async vs non async storage feels irrelevant. Think should be a single type/usage etc. where an optional preloadPromise can be implemented (as implemented on default storage). Not sure if that makes sense. I did something like that on my proposal PR for context #170

I don't care much really about changing anything, just some thoughts.

image

posthog-react-native/src/posthog-rn.ts Outdated Show resolved Hide resolved
posthog-react-native/src/types.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

I think this is good to go

@marandaneto
Copy link
Member Author

I think this is good to go

Teamwork and the community jumping in cc @filipef101
Thanks!

@marandaneto marandaneto merged commit b0086b5 into main Mar 4, 2024
4 checks passed
@marandaneto marandaneto deleted the chore/ref-sync-storage branch March 4, 2024 08:50
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.

Improve/add support for sync providers, ie rn-mmkv
3 participants