-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement EventEmitter
compatible with browsers
#6398
Merged
Muhammad-Altabba
merged 22 commits into
4.x
from
6371-uncaught-typeerror-class-extends-value-undefined-is-not-a-constructor-or-null
Oct 9, 2023
Merged
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
4fb927f
implement `EventEmitter` compatible with browsers
Muhammad-Altabba 0e9ce5d
some fixes at InBrowserEventEmitter
Muhammad-Altabba 28ba5dc
some renaming inside event_emitter.ts
Muhammad-Altabba 87b2b86
Merge branch '4.x' into 6371-uncaught-typeerror-class-extends-value-u…
Muhammad-Altabba ae40b19
Merge branch '4.x' into 6371-uncaught-typeerror-class-extends-value-u…
Muhammad-Altabba 8913307
Merge branch '4.x' into 6371-uncaught-typeerror-class-extends-value-u…
Muhammad-Altabba 6d0bd23
Merge branch '4.x' into 6371-uncaught-typeerror-class-extends-value-u…
Muhammad-Altabba cb742c7
Merge branch '4.x' into 6371-uncaught-typeerror-class-extends-value-u…
Muhammad-Altabba 3766eec
Merge branch '4.x' into 6371-uncaught-typeerror-class-extends-value-u…
Muhammad-Altabba 195463d
export EventEmitter as a class
Muhammad-Altabba 1808058
add unit tests for EventEmitter
Muhammad-Altabba 607f4b2
disable lint rule in a file and update yarn.lock
Muhammad-Altabba 4c8bc1b
apply fixes and add tests for EventEmitter
Muhammad-Altabba e4d7317
configure `EventEmitter` test to run inside `jsdom`
Muhammad-Altabba 9b0ba51
Merge branch '4.x' into 6371-uncaught-typeerror-class-extends-value-u…
Muhammad-Altabba c2eec8e
Merge branch '4.x' into 6371-uncaught-typeerror-class-extends-value-u…
Muhammad-Altabba b51859b
add Cypress configuration to web3-utils
Muhammad-Altabba 7589433
test EventEmitter in the browser with Cypress
Muhammad-Altabba dbbdff2
ignore lint for cypress files at web3-utils
Muhammad-Altabba fe0b27e
Merge branch '4.x' into 6371-uncaught-typeerror-class-extends-value-u…
jdevcs 7697a28
update CHANGELOG.md
Muhammad-Altabba d7e97db
update CHANGELOG.md files
Muhammad-Altabba File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,104 @@ | ||
/* | ||
This file is part of web3.js. | ||
|
||
web3.js is free software: you can redistribute it and/or modify | ||
it under the terms of the GNU Lesser General Public License as published by | ||
the Free Software Foundation, either version 3 of the License, or | ||
(at your option) any later version. | ||
|
||
web3.js is distributed in the hope that it will be useful, | ||
but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
GNU Lesser General Public License for more details. | ||
|
||
You should have received a copy of the GNU Lesser General Public License | ||
along with web3.js. If not, see <http://www.gnu.org/licenses/>. | ||
*/ | ||
|
||
import { EventEmitter as NodeEventEmitter } from 'events'; | ||
|
||
type Callback = (params: any) => void | Promise<void>; | ||
|
||
export class InBrowserEventEmitter extends EventTarget { | ||
private _listeners: Record<string, Callback[]> = {}; | ||
private maxListeners = Number.MAX_SAFE_INTEGER; | ||
|
||
public on(eventName: string, fn: Callback) { | ||
super.addEventListener(eventName, fn as EventListener); | ||
this._addToListeners(eventName, fn); | ||
return this; | ||
} | ||
|
||
public once(eventName: string, fn: Callback) { | ||
const onceCallback = async (params: unknown) => { | ||
await fn(params); | ||
this.off(eventName, onceCallback); | ||
Muhammad-Altabba marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
return this.on(eventName, onceCallback); | ||
} | ||
|
||
public off(eventName: string, fn: Callback) { | ||
super.removeEventListener(eventName, fn as EventListener); | ||
this._removeFromListeners(eventName, fn); | ||
return this; | ||
} | ||
|
||
public emit(eventName: string, params: unknown) { | ||
const event = new CustomEvent(eventName, { detail: params }); | ||
return super.dispatchEvent(event); | ||
} | ||
|
||
public listenerCount(eventName: string): number { | ||
const eventListeners = this._listeners[eventName]; | ||
return eventListeners ? eventListeners.length : 0; | ||
} | ||
|
||
public listeners(eventName: string): Callback[] { | ||
return this._listeners[eventName] || []; | ||
} | ||
|
||
public eventNames(): string[] { | ||
return Object.keys(this._listeners); | ||
} | ||
|
||
public removeAllListeners() { | ||
this._listeners = {}; | ||
return this; | ||
} | ||
|
||
public setMaxListeners(maxListeners: number) { | ||
this.maxListeners = maxListeners; | ||
return this; | ||
} | ||
|
||
public getMaxListeners(): number { | ||
return this.maxListeners; | ||
} | ||
|
||
private _addToListeners(eventName: string, fn: Callback) { | ||
if (!this._listeners[eventName]) { | ||
this._listeners[eventName] = []; | ||
} | ||
this._listeners[eventName].push(fn); | ||
} | ||
|
||
private _removeFromListeners(eventName: string, fn: Callback) { | ||
const eventListeners = this._listeners[eventName]; | ||
if (eventListeners) { | ||
Muhammad-Altabba marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const index = eventListeners.indexOf(fn); | ||
if (index !== -1) { | ||
eventListeners.splice(index, 1); | ||
} | ||
} | ||
} | ||
} | ||
|
||
// eslint-disable-next-line import/no-mutable-exports | ||
export let EventEmitter: typeof NodeEventEmitter; | ||
// Check if the code is running in a Node.js environment | ||
if (typeof window === 'undefined') { | ||
EventEmitter = NodeEventEmitter; | ||
} else { | ||
// Fallback for the browser environment | ||
EventEmitter = InBrowserEventEmitter as unknown as typeof NodeEventEmitter; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this implementation comming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mpetrunic,
This implementation comes from Stackoverflow with the help of ChatGPT 😄 .
Because
events
module is only anode
module that does not exist for the browsers. There is a need to use something different for the browsers. And this class simply uses the classEventTarget
available at the browsers and adds more methods to it which we need in our code.The other alternative to this solution is to use a 3rd party library like: https://www.npmjs.com/package/eventemitter, https://www.npmjs.com/package/events and https://www.npmjs.com/package/eventemitter3. However, I did not want to add an additional dependency.
However, if you have a better approach, kindly advise.
Thanks,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Event emmitters are tricky beasts, easy to cause memory leaks or annoying bugs. I would rely on some solid implementation like https://www.npmjs.com/package/eventemitter3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lib is ~70kb unpack size, for now its good to use some dependable lib but also create issue as lib optimisation in future we may attempt reducing lib size then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mpetrunic and @jdevcs, for your valid concern and good suggestions.
Because these MR changes still rely on node
EventEmitter
, when it is a node environment, there are no changes for node js users. And for the browser, it depends on the available classEventTarget
by just wrapping it to use it similar to nodeEventEmitter
. So, I see this approach as stable. And, actually, our end-to-end tests pass. But I still need to write unit tests.However, as you suggested, I can use
eventemitter3
for now and create a task to write our own Event Emitter later. This is in case you still see the proposed solution in the MR is not good enough even after writing some unit tests. What do you think?[There are failed tests for Firefox. But they are not related to the changes in this MR as they exist in other MRs as well]