feat: prevent fetching toggles when they are up to date #202

jeremiewtd · 2024-05-06T08:46:23Z

About the changes

This pull request is a proposal for fixing this issue to add compatibility with service worker and browser extensions to avoid multiple requests to the proxy server each time the service worker is started.

It introduces a new option togglesStorageTTL that prevents fetching the flags if they are up to date (not older than the number of seconds defined in this option).

A second pull request will come to fix the "refresh" problem described in the issue.

Closes #201

Important files

Main changes concerned index.ts file.

Discussion points

ivarconr · 2024-05-06T12:39:41Z

Thanks for contributing, the idea make sense.
We will try to review it this week.

…-on-load

README.md

jeremiewtd

We made some changes based on your review suggestions, we are waiting for your answers to the remaining questions before doing anything else.

jeremiewtd · 2024-05-16T07:05:20Z

Come to think of it, should the context be part of the timestamp "key"? Otherwise the saved toggles could be wrong if they were saved in a different context than the current starting one in a new instance?

ivarconr · 2024-05-16T11:05:30Z

Come to think of it, should the context be part of the timestamp "key"? Otherwise the saved toggles could be wrong if they were saved in a different context than the current starting one in a new instance?

This makes me think we could store the etag as well. This would yield a validation call to the API but return a 304 if nothing changed. In terms of Unleash/Edge that is a lot cheaper than a full 200 response. Would that be good enough, or do you still see the need to avoid the initial HTTP call?

If we keep the TTL approach we should definitely store a hash of the context object, to make sure the context has not changed in between.

jeremiewtd · 2024-05-24T10:09:22Z

Come to think of it, should the context be part of the timestamp "key"? Otherwise the saved toggles could be wrong if they were saved in a different context than the current starting one in a new instance?

This makes me think we could store the etag as well. This would yield a validation call to the API but return a 304 if nothing changed. In terms of Unleash/Edge that is a lot cheaper than a full 200 response. Would that be good enough, or do you still see the need to avoid the initial HTTP call?

If we keep the TTL approach we should definitely store a hash of the context object, to make sure the context has not changed in between.

The Etag seems like a good idea in absolute terms but it doesn't solve our main problem which is the number of calls to the server. Do you think we should continue with this PR or do you prefer to only use the etag?

ivarconr · 2024-05-24T11:29:34Z

The Etag seems like a good idea in absolute terms but it doesn't solve our main problem which is the number of calls to the server. Do you think we should continue with this PR or do you prefer to only use the etag?

When I think about it feels like two different needs. I would like to keep the TTL separate. In order to test this feature more widely before we commit I would also like to have the option as an experimental option and not part of the official API just yet.

jeremiewtd · 2024-05-24T13:09:49Z

The Etag seems like a good idea in absolute terms but it doesn't solve our main problem which is the number of calls to the server. Do you think we should continue with this PR or do you prefer to only use the etag?

When I think about it feels like two different needs. I would like to keep the TTL separate. In order to test this feature more widely before we commit I would also like to have the option as an experimental option and not part of the official API just yet.

OK, how can we do this experimental option?

ivarconr · 2024-05-24T14:25:09Z

src/index.ts

@@ -53,6 +53,7 @@ interface IConfig extends IStaticContext {
    customHeaders?: Record<string, string>;
    impressionDataAll?: boolean;
    usePOSTrequests?: boolean;
+    togglesStorageTTL?: number;


Instead of adding it directly to the config definition, If we could have an optional "experimental" container we could make it clear that this option might go away or be changed in the future.

experimental?: { togglesStorageTTL?: number; }

Ok we are working on it and adding the hashed context. Do you have any proposal for how you would like this to be done?

Hello

Concerning the hashed context, we started with the use of this type of library : “object-hash”.
Is this OK for you ?

“unleash-proxy-client” lib size => 5.5 KB
“object-hash” lib size => 10 KB

Which is a significant increase in percentage terms.

Last lib update: 02/2022 (v3.0.0),

Otherwise we can simply use a JSON.stringify (more verbose but which does not depend on anything)

We do not want to take on a library of this size for this.

I think the JSON.stringify is a good start, and as this is an experimental feature we can use that to learn the performance impact here. In the future we can consider to take advantage of the built in hasing capabilities, supported by most modern browsers (https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/digest)

Feature/prevent fetch

ivarconr · 2024-07-29T10:32:46Z

Oh, there are a few lint issues. Do you mind running "yarn format" on it to automatically fix these @Florent-Wanteeed @jeremiewtd ?

Tymek

Neat! I'll test it in practical setup. What I also would like to see, maybe later on - not in this PR, is the use of proper hashing. Better not to persist PII when there is no need to do it. JSON.stringify can be a fallback.

Tymek · 2024-07-29T13:40:01Z

src/index.ts

+        const timestamp = Date.now();
+
+        return !!(
+            this.lastRefreshTimestamp &&
+            this.lastRefreshTimestamp <= timestamp &&
+            timestamp - this.lastRefreshTimestamp <=
+                this.experimental.togglesStorageTTL!
+        );


This logic was a bit hard to read

Suggested change

const timestamp = Date.now();

return !!(

this.lastRefreshTimestamp &&

this.lastRefreshTimestamp <= timestamp &&

timestamp - this.lastRefreshTimestamp <=

this.experimental.togglesStorageTTL!

);

const now = Date.now();

const ttl = this.experimental.togglesStorageTTL || 0;

return (

this.lastRefreshTimestamp > 0 &&

this.lastRefreshTimestamp <= now &&

now - this.lastRefreshTimestamp <= ttl

);

Tymek · 2024-07-29T14:15:11Z

src/util.ts

+const sortObjectProperties = (
+    obj: Record<string, unknown>
+): Record<string, unknown> => {
+    const sortedKeys = Object.keys(obj).sort();
+    const sortedObj: Record<string, unknown> = {};
+    sortedKeys.forEach((key) => {
+        if (obj[key] !== null && typeof obj[key] === 'object') {
+            sortedObj[key] = sortObjectProperties(
+                obj[key] as Record<string, unknown>
+            );
+        } else {
+            sortedObj[key] = obj[key];
+        }
+    });
+
+    return sortedObj;
+};


Context has a simple structure. I don't think we need recursion here.
https://docs.getunleash.io/reference/unleash-context#structure

Output doesn't need to directly correspond to context structure.

const sortEntries = (record: Record<string, string>) => { return Object.entries(record).sort(([a], [b]) => a.localeCompare(b)); }; const contextString = (context: IContext): string => { const { properties, ...fields } = context; return JSON.stringify({ fields: sortEntries(fields), properties: sortEntries(context.properties || {}), }); };

src/util.test.ts

Tymek · 2024-07-29T14:27:24Z

src/index.ts

+                if (!response.ok) {
+                    if (response.status === 304) {
+                        this.storeLastRefreshTimestamp();
+                    } else {
+                        console.error(
+                            'Unleash: Fetching feature toggles did not have an ok response'
+                        );
+                        this.sdkState = 'error';
+                        this.emit(EVENTS.ERROR, {
+                            type: 'HttpError',
+                            code: response.status,
+                        });
+
+                        this.lastError = {
+                            type: 'HttpError',
+                            code: response.status,
+                        };
+                    }
+                } else {


Success-first statement is easier to read here, because there is no early return.

if (response.ok) { } else if (response.status === 304) { } else {}

(Using the fact that response.ok is a shorthand)

Tymek · 2024-08-06T08:41:56Z

Merged in #224

jeremiewtd and others added 19 commits April 30, 2024 17:30

to remove: change main file

7d231b0

save last update key after fetch

2fdf1e0

refactor to handle 304 status

4f42346

unit test last update flag

ce45d89

handling isUpToDate

bab5000

update test

a181bd5

fix existing test not working

c9a1677

Store lastRefreshTimestamp to prevent async

3683bed

fix toggleStorageTTL unit

195c438

refactor: rename option

ebcea49

unit test togglesStorage initial fetch

2f46c6b

refactor: emit ready method

ca8ebb9

unit test togglesStorage ready

9463475

unit test ready not sent with bootstrap option

9f6a10c

unit test fetch when expired

cee9f51

documentation

95f126c

improve readme

5ace0d9

restore package.json

c2f34e2

revert public method

cf12da6

jeremiewtd changed the title ~~Prevent fetching flags when they are up to date~~ Prevent fetching toggles when they are up to date May 6, 2024

jeremiewtd changed the title ~~Prevent fetching toggles when they are up to date~~ Feat: prevent fetching toggles when they are up to date May 6, 2024

jeremiewtd added 2 commits May 6, 2024 12:25

restore package

63d22bf

readme

8c921af

jeremiewtd marked this pull request as ready for review May 6, 2024 11:13

ivarconr self-requested a review May 6, 2024 12:33

fix: linting

c225d21

jeremiewtd added 2 commits May 7, 2024 10:02

Merge remote-tracking branch 'origin/main' into feature/prevent-fetch…

54d5ab5

…-on-load

fix: lint

ad853c2

ivarconr reviewed May 7, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

jeremiewtd commented May 10, 2024

View reviewed changes

jeremiewtd added 2 commits May 10, 2024 11:38

doc: update readme

0fc23d1

fix: lint

5bb8c20

ivarconr self-assigned this May 16, 2024

ivarconr reviewed May 24, 2024

View reviewed changes

Florent-Wanteeed added 4 commits June 10, 2024 16:54

experimental and hash

5266140

retour PR

7712d92

Merge branch 'main' into feature/prevent-fetch

4e7a656

rollback previous test optim

7684319

ivarconr changed the title ~~Feat: prevent fetching toggles when they are up to date~~ feat: prevent fetching toggles when they are up to date Jun 24, 2024

Florent-Wanteeed and others added 8 commits June 26, 2024 14:12

Object Hash Value

9ecd585

Merge branch 'main' into feature/prevent-fetch

8aec66c

system time goes back into the past

3c20789

refacto

c0078f8

Met a jour le flag fetchedFromServer quand on est a jour

9e07183

retour

04cd36e

retour PR

2fae8e0

Merge pull request #2 from wanteeed/feature/prevent-fetch

0a5d88c

Feature/prevent fetch

ivarconr requested a review from kwasniew July 25, 2024 08:06

Tymek reviewed Jul 29, 2024

View reviewed changes

Tymek requested a review from Florent-Wanteeed August 5, 2024 08:41

Tymek mentioned this pull request Aug 5, 2024

feat: prevent fetch on load #224

Merged

Tymek closed this Aug 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: prevent fetching toggles when they are up to date #202

feat: prevent fetching toggles when they are up to date #202

jeremiewtd commented May 6, 2024 •

edited

Loading

ivarconr commented May 6, 2024

jeremiewtd left a comment

jeremiewtd commented May 16, 2024

ivarconr commented May 16, 2024

jeremiewtd commented May 24, 2024

ivarconr commented May 24, 2024

jeremiewtd commented May 24, 2024

ivarconr May 24, 2024

jeremiewtd May 31, 2024

Florent-Wanteeed Jun 11, 2024

ivarconr Jun 24, 2024

ivarconr commented Jul 29, 2024

Tymek left a comment •

edited

Loading

Tymek Jul 29, 2024

Tymek Jul 29, 2024

Tymek Jul 29, 2024

Tymek commented Aug 6, 2024

feat: prevent fetching toggles when they are up to date #202

feat: prevent fetching toggles when they are up to date #202

Conversation

jeremiewtd commented May 6, 2024 • edited Loading

About the changes

Important files

Discussion points

ivarconr commented May 6, 2024

jeremiewtd left a comment

Choose a reason for hiding this comment

jeremiewtd commented May 16, 2024

ivarconr commented May 16, 2024

jeremiewtd commented May 24, 2024

ivarconr commented May 24, 2024

jeremiewtd commented May 24, 2024

ivarconr May 24, 2024

Choose a reason for hiding this comment

jeremiewtd May 31, 2024

Choose a reason for hiding this comment

Florent-Wanteeed Jun 11, 2024

Choose a reason for hiding this comment

ivarconr Jun 24, 2024

Choose a reason for hiding this comment

ivarconr commented Jul 29, 2024

Tymek left a comment • edited Loading

Choose a reason for hiding this comment

Tymek Jul 29, 2024

Choose a reason for hiding this comment

Tymek Jul 29, 2024

Choose a reason for hiding this comment

Tymek Jul 29, 2024

Choose a reason for hiding this comment

Tymek commented Aug 6, 2024

jeremiewtd commented May 6, 2024 •

edited

Loading

Tymek left a comment •

edited

Loading