Skip to content

Commit

Permalink
feat: use EvenEmitter3 for web-sdk (#847)
Browse files Browse the repository at this point in the history
<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->

## This PR
<!-- add the description of the PR here -->

Fixes an issue where the `events` node polyfill does not comply to the
`node:events` types.
When trying to use the web OpenFeatureEventEmitter the following error
message comes up, describing that the `events` polyfill's EventEmitter
is incompatible to `node:events` EventEmitter.

```
✘ [ERROR] TS2416: Property 'eventEmitter' in type 'OpenFeatureEventEmitter' is not assignable to the same property in base type 'GenericEventEmitter<ProviderEmittableEvents, Record<string, unknown>>'.
  Type 'EventEmitter' is not assignable to type 'PlatformEventEmitter'.
    Types of property 'addListener' are incompatible.
      Type '(type: string | number, listener: Listener) => EventEmitter' is not assignable to type '(eventName: string | symbol, listener: (...args: any[]) => void) => PlatformEventEmitter'.
        Types of parameters 'type' and 'eventName' are incompatible.
          Type 'string | symbol' is not assignable to type 'string | number'.
            Type 'symbol' is not assignable to type 'string | number'.
```

This PR fixes that issue by not using the `events` anymore and instead
using https://www.npmjs.com/package/eventemitter3

cc @toddbaert 

### Related Issues
<!-- add here the GitHub issue that this PR resolves if applicable -->

Fixes #845

---------

Signed-off-by: Lukas Reining <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Todd Baert <[email protected]>
  • Loading branch information
lukas-reining and toddbaert authored Mar 5, 2024
1 parent 1461074 commit 861cf83
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 38 deletions.
30 changes: 10 additions & 20 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 2 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
},
"devDependencies": {
"@rollup/plugin-typescript": "^11.1.6",
"@types/events": "^3.0.3",
"@types/jest": "^29.5.12",
"@types/node": "^20.11.16",
"@types/react": "^18.2.55",
Expand All @@ -51,7 +50,7 @@
"eslint-plugin-import": "^2.29.1",
"eslint-plugin-jest": "^27.6.3",
"eslint-plugin-jsdoc": "^48.0.6",
"events": "^3.3.0",
"eventemitter3": "^5.0.1",
"jest": "^29.7.0",
"jest-config": "^29.7.0",
"jest-cucumber": "^3.0.1",
Expand All @@ -76,4 +75,4 @@
"packages/react",
"packages/nest"
]
}
}
8 changes: 3 additions & 5 deletions packages/client/src/events/open-feature-event-emitter.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { GenericEventEmitter } from '@openfeature/core';
import { EventEmitter } from 'events';
import { EventEmitter } from 'eventemitter3';
import { ProviderEmittableEvents } from './events';

/**
* The OpenFeatureEventEmitter can be used by provider developers to emit
* events at various parts of the provider lifecycle.
Expand All @@ -9,12 +10,9 @@ import { ProviderEmittableEvents } from './events';
* the result of the initialize method.
*/
export class OpenFeatureEventEmitter extends GenericEventEmitter<ProviderEmittableEvents> {
protected readonly eventEmitter = new EventEmitter({ captureRejections: true });
protected readonly eventEmitter = new EventEmitter();

constructor() {
super();
this.eventEmitter.on('error', (err) => {
this._logger?.error('Error running event handler:', err);
});
}
}
22 changes: 12 additions & 10 deletions packages/shared/src/events/generic-event-emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,15 @@ import { AllProviderEvents, AnyProviderEvent } from './events';
* The GenericEventEmitter should only be used within the SDK. It supports additional properties that can be included
* in the event details.
*/
export abstract class GenericEventEmitter<E extends AnyProviderEvent, AdditionalContext extends Record<string, unknown> = Record<string, unknown>>
export abstract class GenericEventEmitter<
E extends AnyProviderEvent,
AdditionalContext extends Record<string, unknown> = Record<string, unknown>,
>
implements ProviderEventEmitter<E>, ManageLogger<GenericEventEmitter<E, AdditionalContext>>
{
protected abstract readonly eventEmitter: PlatformEventEmitter;

private readonly _handlers: { [key in AnyProviderEvent]: WeakMap<EventHandler, EventHandler[]>} = {
private readonly _handlers: { [key in AnyProviderEvent]: WeakMap<EventHandler, EventHandler[]> } = {
[AllProviderEvents.ConfigurationChanged]: new WeakMap<EventHandler, EventHandler[]>(),
[AllProviderEvents.ContextChanged]: new WeakMap<EventHandler, EventHandler[]>(),
[AllProviderEvents.Ready]: new WeakMap<EventHandler, EventHandler[]>(),
Expand All @@ -33,7 +36,11 @@ export abstract class GenericEventEmitter<E extends AnyProviderEvent, Additional
// The handlers have to be wrapped with an async function because if a synchronous functions throws an error,
// the other handlers will not run.
const asyncHandler = async (details?: EventDetails) => {
await handler(details);
try {
await handler(details);
} catch (err) {
this._logger?.error('Error running event handler:', err);
}
};
// The async handler has to be written to the map, because we need to get the wrapper function when deleting a listener
const existingAsyncHandlers = this._handlers[eventType].get(handler);
Expand Down Expand Up @@ -84,7 +91,7 @@ export abstract class GenericEventEmitter<E extends AnyProviderEvent, Additional
* This is an un-exported type that corresponds to NodeJS.EventEmitter.
* We can't use that type here, because this module is used in both the browser, and the server.
* In the server, node (or whatever server runtime) provides an implementation for this.
* In the browser, we bundle in the popular 'events' package, which is a polyfill of NodeJS.EventEmitter.
* In the browser, we bundle in the popular 'EventEmitter3' package, which is a polyfill of NodeJS.EventEmitter.
*/
/* eslint-disable */
interface PlatformEventEmitter {
Expand All @@ -94,13 +101,8 @@ interface PlatformEventEmitter {
removeListener(eventName: string | symbol, listener: (...args: any[]) => void): this;
off(eventName: string | symbol, listener: (...args: any[]) => void): this;
removeAllListeners(event?: string | symbol): this;
setMaxListeners(n: number): this;
getMaxListeners(): number;
listeners(eventName: string | symbol): Function[];
rawListeners(eventName: string | symbol): Function[];
emit(eventName: string | symbol, ...args: any[]): boolean;
listenerCount(eventName: string | symbol, listener?: Function): number;
prependListener(eventName: string | symbol, listener: (...args: any[]) => void): this;
prependOnceListener(eventName: string | symbol, listener: (...args: any[]) => void): this;
eventNames(): Array<string | symbol>;
}
}

0 comments on commit 861cf83

Please sign in to comment.