launchdarkly / js-core

LaunchDarkly monorepo for JavaScript SDKs
Other
15 stars 19 forks source link

Feedback + Suggestions for React Interface #631

Open leggomuhgreggo opened 1 month ago

leggomuhgreggo commented 1 month ago

Overview

Initially, I intended to suggest aligning the SDK more closely with the existing React SDK's API. However, while drafting this issue, I realized there might not be enough commonality for there to be a clear case for this.

So instead I will just offer some general feedback, as someone who's been using LD on a cross-platform expo project for a few years, and who is also excited for the recent TS-first + Universal React direction y'all are taking.

I should say up front that, while I have thought a bunch about this topic, I come from a place of ignorance, and it's possible I am overlooking tradeoffs. I figure y'all are already on top of things, but hope it's relevant enough to be useful to some degree.

Thanks!

For reference

Current React SDK Hooks The `launchdarkly-react-client-sdk` [exports](https://github.com/launchdarkly/react-client-sdk/blob/main/src/index.ts#L16-L18) the following hooks: ``` useFlags useLDClient useLDClientError ```
Current React Native SDK Hooks The `@launchdarkly/react-native-client-sdk` exports: ``` useBoolVariation useBoolVariationDetail useNumberVariation useNumberVariationDetail useStringVariation useStringVariationDetail useJsonVariation useJsonVariationDetail useTypedVariation useTypedVariationDetail useLDClient ```

Single Flag vs All Flags Hooks

Recap

Preference for Single-Flag Approach

My preference is the single-flag approach, as it offers more granular control and helps avoid unnecessary re-renders. However, there are scenarios where the "all-flags" approach has its benefits—particularly when dealing with feature overrides.

Use Case for All-Flags: Feature Overrides

We use overrides extensively for various purposes, including:

If you have multiple "layers" of overrides, corresponding to different "sources," and you need to manage them as a group, having an "all-flags" hook becomes very useful. This is difficult to achieve with a single-flag interface alone.

Example: Flag Source Layer Priority 1. **overrides > url parameters** — e.g., for e2e testing (also possibly cookies, headers) 2. **overrides > app dev menu** — for demoing and manual validation within the app 3. **overrides > config file** — a convenient `.env`-like file for local development 4. **remote > evaluation** — the variation resolved from LD ⭐ 5. **default > flag fallback** — using `defaultValue` as a fallback argument 6. **defaults > config** — global defaults for all flags, to ensure consistency across references This "maximalist" approach isn't unrealistic

Why Not Just Return All Flags?

An argument could be made that returning all flags simplifies things. However, doing so introduces patterns that can lead to re-render issues. Therefore, I believe the atomic, single-flag approach is more sensible as the primary usage pattern.

That said, having a way to access all flags is a legitimate secondary pattern, especially in scenarios involving overrides.

Type-Specific Hooks?

My impression is that the type-specific hooks are sugar around the useTypedVariation hook — perhaps vestiges of conventions drawn from the more "type-first" native android/ios ecosystems?

I'd suggest that it is more idiomatic to control types via TS, eg

const myFlag = useVariation<boolean>(my-flag-key)

Where the return type, if not specified via generic arg, defaults to a union of the possible options

type FlagReturnType = boolean | number | string | JSON | undefined

I think this simplifies the coupling to default value in resolving type [code]

And it has the advantage of reducing the surface area and making adoption a friendlier proposition.

Consider: Standard React Hooks

If we consider the standard react hooks, they enable types through generics/overloads — but still leave this optional.

This makes it easy to have strong typing, but still leaves it accessible for teams that aren't as strict about TS observances.

Here's the TS definition for useState as an example.

    /**
     * Returns a stateful value, and a function to update it.
     *
     * @version 16.8.0
     * @see {@link https://react.dev/reference/react/useState}
     */
    function useState<S>(initialState: S | (() => S)): [S, Dispatch<SetStateAction<S>>];
    // convenience overload when first argument is omitted
    /**
     * Returns a stateful value, and a function to update it.
     *
     * @version 16.8.0
     * @see {@link https://react.dev/reference/react/useState}
     */
    function useState<S = undefined>(): [S | undefined, Dispatch<SetStateAction<S | undefined>>];

Default Value Usage?

This is a nuanced question — but let's consider the differences between these approaches:

const myFlag = useVariation<boolean>(my-flag-key) ?? false

vs

const myFlag = useVariation<boolean>(my-flag-key, false)

The fist delegates defaultValue to user-land

The second makes the defaultValue a part of the standard API even a requirement, in the current implementation

Global Defaults?

Another option is to provide a "flag defaults" config to the LD init — so that the default for a given flag is consistent across usages.

Suggestion

I'd suggest that this is probably the ideal usage pattern / interface ⭐

useFlag(key: FlagKeys | string, defaultValue?: FlagTypes) : FlagTypes | undefined

Personally, I would not use the defaultValue argument — preferring instead to have global defaults — but it's there for those who prefer it.

Async vs Sync

There are certain situations where it's essential to resolve an up-to-date value from LD, and not use the cached/default version.

Currently the way to handle this is something like:

const useMyFeatureFlag = () => {
  const { isReady } = useLDClient();

  if !isReady return undefined; // or whatever default value

  const myKeyValue = useFlag('my-key');

  return myKeyValue
}

const MyComponent = () => {
  const myFeatureFlag = useMyFeatureFlag

  if (!myFeatureFlag) return LoadingView

  return <FeatureComponent flagValue={myFeatureFlag} />
}

However, this approach can be cumbersome.

I'd suggest this usage indicates adding support for a sort of async style interface, similar to modern API client libraries like react-query and urql

const MyComponent = () => {
  const [ myFeatureFlag, reQueryVariation ] = useFlagAsync('my-key'); // alternatively useFlagQuery?

  if (myFeatureFlag.fetching) return LoadingView
  if (myFeatureFlag.error) return ErrorView

  return <FeatureComponent flagValue={myFeatureFlag.value} />
}

Indeed it may even be worth adding support for "request policy" eg (from urql docs)

  • cache-first (the default) prefers cached results and falls back to sending an API request when no prior result is cached.
  • cache-and-network returns cached results but also always sends an API request, which is perfect for displaying data quickly while keeping it up-to-date.
  • network-only will always send an API request and will ignore cached results.
  • cache-only will always return cached results or null.

Future Consideration: Codegen?

It would be great to generate the type definitions for flags, as part of the dev/ci workflow.

Of course, there's nothing stopping users from doing this currently, but it would be great if there were official feature support. (perhaps it's already on the roadmap!)

I mention this because I think this possible eventuality might inform design choices around type handling — and also possibly generated default values.

kinyoklion commented 1 month ago

Hello @leggomuhgreggo,

Thank you for the feedback. For some of these items I can provide background and insight.

The main thing that will drive a number of these items is driving LaunchDarkly features. The evaluated value of a flag is what you consume in your application, but the side-effects of those evaluations drive features (experimentation, flag insights, stale flags, fallback values, etc.). Some aspects of the interface allow these features to be driven better than they were in the previous react SDK.

The first item is heavily influenced by this. The old react SDK useFlags caused the SDK to have many problems working with features, especially experimentation. Initially it wouldn't drive these features at all, because it had no way to know that a specific flag was evaluated for a specific context.

When you evaluate a flag we drive features by knowing:

Eventually we figured out complex ways to get this data from useFlags (other than defaults.) What we had to do was implement an HTML5 proxy that was returned in place of the actual flag values. This proxy would then intercept access to the flags. This solution isn't permanent though, and it certainly isn't expected, but it was required to make it work.

In the new SDK we removed all flags from the path of least resistance and instead provide individual flags as the path of least resistance. You still can get all the flags from the client, but doing so means you need to be aware of the potential problems and features that do not work.

This is also the answer to another of the questions (flag, default) versus (flag) ?? default. If the default is handled in the application, instead of via the SDK, then that information is missing from analytics.

For the async/sync aspects, there it may be unclear what the SDK does. We connect to LaunchDarkly and receive updates as they happen. If a flag changes then the hook result will be different and changes propagate. There is a "stale" period between identify of one context and the next context. If you prefer the most recent results, once that identify completes, then you can use the waitForNetworkResults option when the identify happens. In that case we load the cache, but don't return from the identify until we have reconnected, or until we have failed to connect. This cannot be controlled at a per flag level, because the flags are not requested like an HTTP request. The cache control policy for the individual identify an be controlled though, currently with waitForNetworkResults, but other kinds of behavior could be added.

As flag values change the various hooks will propagate those changes.

Another option is to provide a "flag defaults" config to the LD init — so that the default for a given flag is consistent across usages.

Yes, we generally call this bootstrap. It is currently being developed for our browser implementation which shares much of the RN code. The identify call in this case will take a bootstrap and that will be used for all calls until the identify completes (or just use them). Typically bootstrap would be generated by one of our server SDKs, but it doesn' strictly have to be. (You could build this bootstrap at CI time, or you could do it during login of an application, for example.)

In regard to the typing of the useVariation methods, this partially fits under the "The detailed reasons for the evaluation." as well as how we handle default values. It also has to do with JavaScript versus typescript.

When you write generic methods the typing information exists at compile time, but it is erased at runtime. The type of the individual flag is only known at runtime. When you used typed variation methods we return the default value if the type at runtime does not match the expected type. We also include this information into analytics events.

These methods also allow a boolean typed evaluation, with defaults and alanytics events, while using JavaScript. Sometimes in JavaScript you want the value to actually be a specific type, and you want the default when it is not that type.

I agree that some code generation would be useful, that wouldn't be a feature of this SDK specifically though. For requesting that type of product feature our support process is a better avenue.

Thank you, Ryan

kinyoklion commented 1 month ago

An additional note on the "up to date value". The identify promise is the thing that indicates that the operation is complete. With the caching behavior controlled by waitForNetworkResults if you want to attempt to get the latest value before completing the operation. The values are tied to the identify operation. We get them all at once and don't make any query on a specific variation call. So after that identify is complete we will usually have the values. We won't always though, phones don't always have the internet (or it isn't always good), so we will timeout, and then you get cached or default values.

Because the identify operations can be asynchronous to other things that are happening I am unsure how we would bring this to the level of an individual evaluation. We don't know the relationship between the two operations.

For instance imagine you have this set of operations.

In your auth service: await - identify anonymous context await - identify logged in user

In some UI component: const res = useVariationSomeNewThing(flag, default).

Which operation does the useVariationSomeNewThing wait for? The application managing the relationship of the identify promise, versus the component, would determine this.

There are many "correct" options.

The case you demonstrated was just whatever one happens to complete, but that isn't always the situation. One option may be to provide a something like useVariationResolved(flag, default, promise) where the specific promise cascades, but it would take some consideration.

Identify is very similar to managing a login in many applications. The UI may be different, and parts may be gated, until the login is complete. In such applications it is easiest to potentially re-use the mechanism already propagated for auth. Or to make your own hook that informs you of the state you are concerned with.

Thanks again, Ryan