Unleash / unleash-proxy-client-js

A browser client that can be used together with the unleash-proxy.
Apache License 2.0
47 stars 46 forks source link

Dynamic refresh interval #133

Closed disjukr closed 1 year ago

disjukr commented 1 year ago

Describe the feature request

const wait = (sec) => new Promise(resolve => setTimeout(resolve, sec * 1000));

let i = 0;

const client = new unleash.UnleashClient({
  appName,
  url,
  clientKey,
  refreshInterval: () => wait(++i), // refresh after 1 sec, 2 sec, 3, 4...
});

https://github.com/Unleash/unleash-proxy-client-js/blob/d5f0c743/src/index.ts#L31

  interface IConfig extends IStaticContext {
      url: URL | string;
      clientKey: string;
      disableRefresh?: boolean;
-     refreshInterval?: number;
+     refreshInterval?: number | () => Promise<void>;
      metricsInterval?: number;
      disableMetrics?: boolean;
      storageProvider?: IStorageProvider;
      context?: IMutableContext;
      fetch?: any;
      bootstrap?: IToggle[];
      bootstrapOverride?: boolean;
      headerName?: string;
      customHeaders?: Record<string, string>;
      impressionDataAll?: boolean;
      usePOSTrequests?: boolean;
  }

Background

We want to refresh values more frequently when user uses the app actively (we will check the activity from keyboard & mouse event). Currently we can't configure refresh rate dynamically so I propose new config interface setting next timeout interval.

Solution suggestions

No response

thomasheartman commented 1 year ago

Hey, @disjukr! Thanks for the feature request. It's an interesting idea for sure. But out of curiosity, why would you use a function that returns Promise<void>? Wouldn't it be easier to just return the number itself? A () => number? Would that miss any use cases?

Anyway, I'll bring this to the team and see what they think. Would you be willing to work on a PR for it?

disjukr commented 1 year ago

Thank you for considering this feature. I don't have a plan for make PR for now. Why return promise rather than number: I want to do something like below (not exactly but likely).

const client = new unleash.UnleashClient({
  appName,
  url,
  clientKey,
  refreshInterval: () => Promise.all([
    wait(3),
    detectUserActivity(),
  ]),
});

const wait = (sec) => new Promise(resolve => setTimeout(resolve, sec * 1000));

function detectUserActivity() {
  return new Promise((resolve) => {
    const handler = () => {
      resolve();
      removeEventListener('mousemove', handler);
      removeEventListener('keydown', handler);
    };
    addEventListener('mousemove', handler);
    addEventListener('keydown', handler);
  });
}
thomasheartman commented 1 year ago

Ah, I see! So only have the client refresh if it's been at least 3 seconds and the user has interacted with the application?

But let me challenge the idea a bit: does this serve an actual need? Do you have any use cases where increasing the refresh rate serves a purpose? It's definitely a fun idea, but I'm not sure I what value it brings. I suppose I could see a use case if you want to eliminate any redundant calls being made, but in that case it might be easier to set refresh rate to 0 instead.

Curious to hear your thoughts ☺

disjukr commented 1 year ago

AFAIK, if the refresh rate is set to 0, the refresh doesn't work at all and It's not I want. Actually the I want is refresh instantly when server state is changed but I also don't want additional complexity of infra such as message queue to support websocket or SSE to implement that. So what I need is polling in the client as smart as possible.

When the refresh rate is set tight, it may not mean much as you said. But if is set so loose, I think we may want to cancel it while waiting. The code that will actually go into our application is probably closer to the below.

const client = new unleash.UnleashClient({
  appName,
  url,
  clientKey,
  refreshInterval: () => Promise.all([
    wait(1), // To prevent calling too frequently
    Promise.race([
      wait(dynamicInterval), // 1 ~ 60. Adjusted according to the amount of recent activity
      detectUserActivity(), // Debounce may be needed to prevent the screen from changing during user activity
    ]),
  ]),
});
thomasheartman commented 1 year ago

Yeah, correct. Setting it to 0 means you'll only fetch toggles on startup. While this may be the right approach in some apps, there's also cases that need to refresh every now and then.

Thanks for the clarification too! I'm still not sure that modifying the interval will make that much of a difference. Does it really matter whether a user sees a feature one minute earlier or one minute later?

thomasheartman commented 1 year ago

Oh, and there is also the thing about the refresh interval of the proxy: Assuming you're connecting to the Unleash proxy, it doesn't matter how often this SDK tries to refresh if the Proxy's refresh rate is lower. Of course, you could have the proxy constantly update, but that's probably going to cause a lot of useless http requests.

disjukr commented 1 year ago

Does it really matter whether a user sees a feature one minute earlier or one minute later?

Because the experience of the person toggling the flag is also important. It can be frustrating to have to wait several seconds after toggling a flag to see if it's applied properly, if the page loading speed is too slow to simply refresh the entire page (or if it's difficult to quickly reach the screen state they were viewing before).

Assuming you're connecting to the Unleash proxy, it doesn't matter how often this SDK tries to refresh if the Proxy's refresh rate is lower.

Oh it's bad news to me if unleash proxy refresh rate is longer than few seconds 😔

thomasheartman commented 1 year ago

Because the experience of the person toggling the flag is also important. It can be frustrating to have to wait several seconds after toggling a flag to see if it's applied properly

Yeah, I agree with this. Flipping a switch and then waiting for it to show up can be confusing. It can be hard to know whether it's active yet or if it's just something you've done wrong.

But if you need this kind of instant visual feedback, is there a way to make it work for you? Could you for instance have a local version of the app open with very frequent refreshes? You could maybe even have a separate instance of the proxy that you use specifically for cases like this and that updates very frequently?

Oh it's bad news to me if unleash proxy refresh rate is longer than few seconds 😔

If you're hosting the proxy yourself, you can set the refresh rate to whatever you want (the default is 5 seconds). So if you wanted it to update every second, you could do that, but that'd mean a lot of requests, so I wouldn't necessarily recommend it if you're deploying lots of them.


In general, making client-side SDKs update immediately is tricky because of the layer of indirection they use (the proxy or the front-end API), I'm afraid 🤷🏼

disjukr commented 1 year ago

Thanks for listening to my request and answering. Although I posted a suggestion, I'm not passionate enough to make this feature outright.

Since our team is running a self-hosted version of Unleash, maybe we could make the refresh rate of the Unleash proxy shorter, but I probably wouldn't ask an infrastructure engineer for that.

thomasheartman commented 1 year ago

Of course 😄 And no worries! I'll leave the issue open for now. If anyone wants to discuss it further or pick it up, they're welcome to. If nothing happens for a while, stale bot will come and close it for now.

I think what you're asking for is a legitimate use case, but Unleash has decided to opt for an eventual constistency-model (which is also why we've never focused on streaming), because it covers our needs in more or less every business case that we can think of. Having to wait slightly longer to see a feature become active is a cost that we accept at the moment.

But of course, it affects DX, so if we can find a way to improve that, then we're very interested. So with that in mind, we're open to discussing implementations if anyone wants to develop it with us.

As an alternate suggestion, could you just manually fetch toggles when you want to? By default it uses the "refreshInterval", but you could force an update by calling updateContext or setContextField. If that solves your issue, we might be open to creating a public method on the client called refreshToggles or something to simply force an update?

disjukr commented 1 year ago

By default it uses the "refreshInterval", but you could force an update by calling updateContext or setContextField. If that solves your issue, we might be open to creating a public method on the client called refreshToggles or something to simply force an update?

Oh great, it will be enough to turn off refreshInterval and create one myself with refreshToggles or something. I didn't know there are updateContext and setContextField exist.

thomasheartman commented 1 year ago

Super! In that case, I'll close this issue for now. But let me know how it works out! If you find that it's essential for you to force a refresh of the toggles, then I can imagine that that might be something we'd be willing to build at some point ☺️