optimizely / react-sdk

React SDK for Optimizely Feature Experimentation and Optimizely Full Stack (legacy)
https://docs.developers.optimizely.com/experimentation/v4.0.0-full-stack/docs/javascript-react-sdk
Apache License 2.0
89 stars 35 forks source link

Add `react-native-mmkv` support #166

Open anton-patrushev opened 2 years ago

anton-patrushev commented 2 years ago

Is the any plans to add support for react-native-mmkv storage library instead of async-storage package for React Native environment?

opti-jnguyen commented 2 years ago

Thanks for bringing this up @anton-patrushev ! We have no plans at the moment to add support for react-native-mmkv instead of async-storage.

Will bring this topic up to the team for discussion!

In the meantime, if you'd like to outline your use case for react-native-mmkv that would help us determine how we will prioritize it - thanks in advance!

mikechu-optimizely commented 1 year ago

I've created a research spike ticket (FSSDK-9621) to look into swapping to react-native-mmkv. I can't see supporting both.

The package does seem to have quite a bit of performance advantages over react-async-storage.

On the other hand, we're currently resolving tickets around the package (and bundled) size of our JS and React SDKs.

We'll have to balance all the pros & cons. Thanks for the suggestion.

anton-patrushev commented 8 months ago

Thanks @opti-jnguyen @mikechu-optimizely. That makes sense to me. Thanks for doing a research spike! I thought the MMKV package had a reasonable performance advantage to migrate to it as the standard instead of react-async-storage. But sounds like this could be outdated information. And it seems like async storage is also a JSI TurboModule nowadays, so it's fine to keep it from the performance perspective.

But I have a different question now:

Do you plan to add an optional flexible storage adapter param to the optimizely instance configuration, so the developers of the host app can decide and implement it based on their needs?

So instead of declaring async storage as the peer dependency (or maybe still doing that, but make sure host app installation would not fail if the async storage is missing) you can interact with some abstract parameter implementing the StorageAdapter interface.

Example:

import { createInstance, StorageAdapter } from '@optimizely/react-sdk'
// StorageAdapater type/interface is exposed from the optimizely SDK package

const storageAdapter: StorageAdapter  = {
  get: () => {},
  set: () => {},
  delete: () => {},
  update: () => {},
}; // complexity of this `storageAdapter` logic depends on your internal needs of course

// ...

createInstance({
  sdkKey: '',
  storageAdapter,
});
mikechu-optimizely commented 8 months ago

That's a good idea.

We'll stick with the current async-storage as a Default and providing DI for an adapter as long as it conforms to an interface. It'll take some doing / version coupling to traverse across the JS/TS SDK up here to React SDK.

If you'd like to contribute a solution the way you see it, feel free to open a PR in the JS lib or here.

I've updated the Spike ticket (we're behind in processing).

anton-patrushev commented 7 months ago

Thanks @mikechu-optimizely

Appreciate rapid responses from your side and my apologizes for slow responses 🙃 Do you have any updates?

mikechu-optimizely commented 7 months ago

Hey.

We have slated to work through the backlog of JS & React tickets over the next 1–2 sprints. I've pinged my manager through the aforementioned spike ticket to see where we are with possibly adding an adapter pattern as a way to let implementers decide how to handle local storage

anton-patrushev commented 7 months ago

that would be awesome if you would kick it off! thanks @mikechu-optimizely

mikechu-optimizely commented 7 months ago

I've put it as an agenda item for our stand-up meeting today.

mikechu-optimizely commented 7 months ago

My PM put this work into the next sprint (current one is packed). I've put this on my calendar to follow up on.

anton-patrushev commented 6 months ago

Any updates @mikechu-optimizely ?

mikechu-optimizely commented 6 months ago

Hi. Yes. This ticket is in our current sprint (4-15 Mar) and actively being researched.

mikechu-optimizely commented 6 months ago

@anton-patrushev I'm reviewing the PR for this request.

Was this what you had in mind?

mikechu-optimizely commented 6 months ago

This is being released in JS SDK v5.2.0

anton-patrushev commented 6 months ago

I was looking into PR, not sure if I found exactly what I want, but I saw some new params in the optimizely instance config that was called persistentCacheProvider or something like that, so I believe it's what I'm looking for.

And if it would allow us to remove async-storage and replace it with our custom definition and implementation of PersistentCacheProvider interface based on our own local storage solution (most likely mmkv in our case) — that's awesome!

Thanks!

Lemme take a look in it this week (would try to upgrade optimizely SDK on our project) and I would let your know feedback and we can close an issue after that!

mikechu-optimizely commented 6 months ago

I'm trying to button up another React issue to publish React SDK with the JS SDK 5.2 dep.

anton-patrushev commented 6 months ago

Awesome, thanks @mikechu-optimizely lemme know here please when the new React SDK will be published so I can try to upgrade and migrate to another storage solution on my end