Removed NodeJS event emitter in common library. #330
Conversation
…ill run all the steps
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -7,7 +7,7 @@ | |||
| "module": "build/pixelstreamingjsstreamer.js", | |||
| "scripts": { | |||
| "develop": "webpack-dev-server --mode development", | |||
| "build": "tsc --project tsconfig.json && webpack --mode production", | |||
| "build": "tsc --project tsconfig.json && webpack --mode development", | |||
There was a problem hiding this comment.
Didn't really make sense to minify/obscure this code as it is only used in dev.
| @@ -45,7 +45,7 @@ module.exports = { | |||
| }, | |||
| }, | |||
| static: path.join(__dirname, 'dist'), | |||
| compress: true, | |||
| compress: false, | |||
There was a problem hiding this comment.
Didn't make sense to compress this code as we only use it for testing/dev.
| @@ -581,7 +581,7 @@ export type PixelStreamingEvent = | |||
| | PlayerCountEvent | |||
| | WebRtcTCPRelayDetectedEvent; | |||
|
|
|||
| export class EventEmitter extends EventTarget { | |||
| export class PixelStreamingEventEmitter extends EventTarget { | |||
There was a problem hiding this comment.
We already had a class called EventEmitter, so I renamed it here. This seemed like the lesser impactful rename as opposed to renaming the one in common.
| @@ -4,13 +4,14 @@ | |||
| "description": "", | |||
| "main": "index.js", | |||
| "scripts": { | |||
| "test": "npx playwright test" | |||
| "test": "npx playwright test", | |||
| "build": "" | |||
There was a problem hiding this comment.
You may notice a few empty build scripts in this commit.
Those were added to enable running npm run build in the root of the repo and building all workspaces - handy.
| @@ -1,6 +1,7 @@ | |||
| # Use the current Long Term Support (LTS) version of Node.js | |||
| FROM node:lts | |||
| # Copy the signalling server and frontend source code from the build context | |||
| COPY package.json package.json | |||
There was a problem hiding this comment.
This was needed in the Dockerfile to make it use the new NPM workspaces way of doing things.
| "private": true, | ||
| "scripts": { | ||
| "build": "npm run build --ws" | ||
| } |
There was a problem hiding this comment.
This lets us run npm run build in the repo root and build everything.
…hanges that were not needed.
| this._callback = callback; | ||
| } | ||
|
|
||
| handleEvent(_evt: Event): void { |
There was a problem hiding this comment.
Should the event not be passed to the callback?
There was a problem hiding this comment.
Still questioning this one.
There was a problem hiding this comment.
passing that Event to the downstream listener would actually introduce different behaviour than what node js was doing with its event emitter
It has no meaningful information attached to it and is really an internal implementation detail, exposing it to the listener would be a leaky abstraction considering the point was the make something that looks and smells likes NodeJS event emitter but actually uses EventTarget under the hood.
Relevant components:
Problem statement:
The
Commonlibrary contained a NodeJS type,EventEmitterthis was causing it to fail when used in the browser. This was not a problem previously when we bundled at the library build step, but since #324 have been a few dependencies that do not work well in the browser without bundling.This issue was reported in: #328 and impacts the latest version of UE 5.5 frontend library.
Solution
This PR solves the issue by making a browser compatible EventEmitter type using EventTarget under the hood.
Documentation
N/A
Test Plan and Compatibility
I have tested by: