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

Require only the used subset of AsnycStorage as custom storage #246

Merged
merged 1 commit into from
May 10, 2024

Conversation

bfricka
Copy link

@bfricka bfricka commented May 1, 2024

According to the implementation in javascript/mixpanel-storage.js, only a subset of the AsyncStorage interface is actually used by Mixpanel.

Not having this interface explicitly defined is problematic, because it requires devs to assume that Mixpanel may use a larger subset of the interface at any time. Devs would need to either implement the full interface, or risk breaking changes if Mixpanel's usage changes. Additionally, the types should reflect the required interface, so it's obvious that they have appropriately implemented it.

This PR does two main things:

  1. Defines that interface in the official types. So devs can be sure they are implementing the full interface required by Mixpanel
  2. References this interface in the docs

Additionally, it links to the correct AsyncStorage reference, rather than the one that has been long removed from React Native.

- Define the subset of `AsnycStorage` as `MixpanelAsyncStorage` in types
- Update docs to reference this subset
@bfricka
Copy link
Author

bfricka commented May 1, 2024

Note that, the fact that Mixpanel only uses a subset of the interface is fortuitous, as it allows the common WebStorage interface to simply be async wrapped, e.g.:

const wrapAsync = <R>(fn: () => R) => {
  return new Promise<R>((resolve, reject) => {
    try {
      resolve(fn())
    } catch (e) {
      reject(e)
    }
  })
}

export const asyncWebStorage = {
  getItem: (key) => wrapAsync(() => localStorage.getItem(key)),
  setItem: (key, value) => wrapAsync(() => localStorage.setItem(key, value)),
  removeItem: (key) => wrapAsync(() => localStorage.removeItem(key)),
} satisfies MixpanelAsyncStorage

Or in our case, we're wrapping react-native-mmkv

@zihejia
Copy link
Contributor

zihejia commented May 4, 2024

Thank you so much @bfricka !

Copy link
Contributor

@zihejia zihejia left a comment

Choose a reason for hiding this comment

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

LGTM

@zihejia zihejia merged commit ee7e8bb into mixpanel:master May 10, 2024
2 of 3 checks passed
@zihejia zihejia added the enhancement New feature or request label May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants