[FFM-1500] ability to listen on ff events #23

enver-bisevac · 2021-10-15T11:51:43Z

Now you can use event listeners:

on(Event.READY, () => {
  console.log('READY');
});

on(Event.FAILED, () => {
  console.log('FAILED');
});

on(Event.CHANGED, (identifier) => {
  console.log('Changed', identifier);
});

knagurski · 2021-10-18T15:13:31Z

README.md

+- Event.READY - SDK successfully initialized
+- Event.FAILED - SDK throws an error
+- Event.CHANGED - any new version of flag or segment triggers this event, if segment is changed then it will find all flags with segment match operator


Probably worth formatting these as code

Suggested change

- Event.READY - SDK successfully initialized

- Event.FAILED - SDK throws an error

- Event.CHANGED - any new version of flag or segment triggers this event, if segment is changed then it will find all flags with segment match operator

- `Event.READY` - SDK successfully initialized

- `Event.FAILED` - SDK throws an error

- `Event.CHANGED` - any new version of flag or segment triggers this event, if segment is changed then it will find all flags with segment match operator

knagurski · 2021-10-18T15:13:48Z

README.md

+
+Methods:
+
+```


Suggested change

```

```typescript

knagurski · 2021-10-18T15:14:26Z

README.md

+});
+```
+
+and if you want to remove functionReference listener for Event.READY:


Suggested change

and if you want to remove functionReference listener for Event.READY:

and if you want to remove the `functionReference` listener for `Event.READY`:

knagurski · 2021-10-18T15:14:47Z

README.md

+off(Event.READY, functionReference);
+```
+
+or if you want to remove all listeners on Event.READY:


Suggested change

or if you want to remove all listeners on Event.READY:

or if you want to remove all listeners on `Event.READY`:

knagurski · 2021-10-18T15:15:36Z

README.md

+off(Event.READY);
+```
+
+or if you call off() without params it will close the client.


Suggested change

or if you call off() without params it will close the client.

or if you call `off()` without params it will close the client.

knagurski · 2021-10-18T15:17:09Z

example/cf_reactive.mjs

+  console.log('READY');
+});
+
+CfClient.on(Event.FAILED, () => {


Does this listener get an indication as to why it failed?

make sense, I will add argument

knagurski · 2021-10-18T15:39:49Z

src/client.ts

+    }
+  }
+
+  off(event: string, callback: () => void): void {


It looks like event and callback are required.

Suggested change

off(event: string, callback: () => void): void {

off(event?: string, callback?: () => void): void {

make sense :)

knagurski · 2021-10-18T15:41:37Z

src/index.ts

+  on: function (event: Event, callback: (...args: unknown[]) => void): void {
+    this.instance.on(event, callback);
+  },
+  off: function (event: Event, callback: () => void): void {


Suggested change

off: function (event: Event, callback: () => void): void {

off: function (event?: Event, callback?: () => void): void {

knagurski · 2021-10-18T15:47:13Z

src/repository.ts

+    for (const key of keys) {
+      const flag = await this.getFlag(key);
+      if (!flag) {
+        return [];
+      }
+      for (const rule of flag?.rules) {
+        for (const clause of rule?.clauses) {
+          if (
+            clause.op === SEGMENT_MATCH_OPERATOR &&
+            clause.values.includes(segment)
+          ) {
+            result.push(flag.feature);
+          }
+        }
+      }
+    }


I wonder if this could be a bit more efficiently with filter and reduce?

knagurski · 2021-10-18T15:49:57Z

src/repository.ts

+    }
+    for (const key of keys) {
+      const flag = await this.getFlag(key);
+      if (!flag) {


So if any key doesn't match a flag, we return a blank array? Curious if it would make sense to continue instead?

knagurski · 2021-10-18T15:52:45Z

src/repository.ts

  private async isFlagOutdated(
    key: string,
    flag: FeatureConfig,
  ): Promise<boolean> {
    const oldFlag = await this.getFlag(key, false); // dont set cacheable, we are just checking the version
-    return oldFlag?.version && oldFlag.version > flag?.version;
+    return oldFlag?.version && oldFlag.version >= flag?.version;


Do we want to consider a flag outdated if it's the same version as the old one?

on any change it should change version right? if I take out this then it will trigger the change event every time when poller is working and SSE breaks, because poller will poll in every 60 sec. The only way to avoid this is to use diff between old and new object? What do you think? Maybe @davejohnston can suggest something, but I think version should be always increased on every flag or segment change

knagurski · 2021-10-18T15:52:57Z

src/repository.ts

  }

  private async isSegmentOutdated(
    key: string,
    segment: Segment,
  ): Promise<boolean> {
    const oldSegment = await this.getSegment(key, false); // dont set cacheable, we are just checking the version
-    return oldSegment?.version && oldSegment.version > segment?.version;
+    return oldSegment?.version && oldSegment.version >= segment?.version;


knagurski · 2021-10-28T09:34:38Z

README.md

+
+and if you want to remove the `functionReference` listener for `Event.READY`:
+
+```


Might as well add the language to all code blocks

Suggested change

```

```typescript

knagurski · 2021-10-28T09:34:54Z

README.md

+
+or if you want to remove all listeners on `Event.READY`:
+
+```


Suggested change

```

```typescript

knagurski · 2021-10-28T09:35:27Z

example/cf_reactive.mjs

+CfClient.on(Event.FAILED, (error) => {
+  console.log('FAILED with err:', error);
+});


knagurski · 2021-10-28T09:41:09Z

src/client.ts

+    }
+  }
+
+  on(event: Event, callback: (...args: unknown[]) => void): void {


One for another time, but it would be nice to be able to type the callback arguments based on the event type. Should make the DX a bit nicer. You can probably do that best with overloading, but I don't have a lot of experience in that area in TS. More than fine as-is for this PR, just thinking out loud

knagurski

Awesome work! Sorry I kept forgetting to look at it again!

enver-bisevac requested a review from knagurski October 15, 2021 11:51

enver-bisevac mentioned this pull request Oct 15, 2021

Ability to Listen to Events #19

Closed

knagurski reviewed Oct 18, 2021

View reviewed changes

enver-bisevac requested a review from knagurski October 19, 2021 08:10

enver-bisevac added 4 commits October 21, 2021 15:53

listening on events, works also when there is no SSE connection

0948eda

updated docs, minor changes

75511f3

minor fix

bb29474

requested changes

deb5efe

enver-bisevac force-pushed the FFM-1500 branch from d11774b to deb5efe Compare October 21, 2021 14:21

requested changes

531205c

knagurski reviewed Oct 28, 2021

View reviewed changes

README.md

or if you want to remove all listeners on `Event.READY`:

```

Copy link

Contributor

knagurski Oct 28, 2021

Choose a reason for hiding this comment

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

Suggested change

```

```typescript

knagurski reviewed Oct 28, 2021

View reviewed changes

knagurski approved these changes Oct 28, 2021

View reviewed changes

enver-bisevac merged commit ecd76dd into main Oct 28, 2021

enver-bisevac deleted the FFM-1500 branch October 28, 2021 09:44

	and if you want to remove functionReference listener for Event.READY:
	and if you want to remove the `functionReference` listener for `Event.READY`:

	or if you want to remove all listeners on Event.READY:
	or if you want to remove all listeners on `Event.READY`:

	or if you call off() without params it will close the client.
	or if you call `off()` without params it will close the client.

	off(event: string, callback: () => void): void {
	off(event?: string, callback?: () => void): void {

	off: function (event: Event, callback: () => void): void {
	off: function (event?: Event, callback?: () => void): void {


		Methods:

		```

[FFM-1500] ability to listen on ff events #23

[FFM-1500] ability to listen on ff events #23

Uh oh!

Conversation

enver-bisevac commented Oct 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knagurski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!