Skip to content
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

fix: remove CustomEvent polyfill #2652

Merged
merged 1 commit into from
Aug 9, 2024
Merged

Conversation

EliasOenal
Copy link
Contributor

@EliasOenal EliasOenal commented Aug 7, 2024

Description

The EventTarget implementation of libp2p contained workarounds for incomplete runtime support for the feature. One of the workarounds was specifically marked to be removed once CustomEvent was implemented in NodeJS and the upstream ticket was closed. This has happened two years ago. The implementation of the standard event in Node18 and the subsequent removal of the experimental flag in Node19 broke the current workaround and causes tsc to error. Some of these were suppressed with @ts-ignore, others were not. This fix closes #2420.

The fix removes the workaround as instructed by the source code and restores compatibility with recent versions of Node:

/**
 * CustomEvent is a standard event but it's not supported by node.
 *
 * Remove this when https://github.com/nodejs/node/issues/40678 is closed.
 *
 * Ref: https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent
 */
class CustomEventPolyfill<T = any> extends Event {
  /** Returns any custom data event was created with. Typically used for synthetic events. */
  public detail: T

  constructor (message: string, data?: EventInit & { detail: T }) {
    super(message, data)
    // @ts-expect-error could be undefined
    this.detail = data?.detail
  }
}

export const CustomEvent = globalThis.CustomEvent ?? CustomEventPolyfill

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
    • I don't believe documentation is needed.
  • I have added tests that prove my fix is effective or that my feature works
    • I have not been able to setup the test environment on my MacOS, but the tests in my personal libp2p application pass. Please test the fix against the libp2p tests as well.

@EliasOenal EliasOenal requested a review from a team as a code owner August 7, 2024 07:29
Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

CustomEvent was added to node.js in v18.7.0.

electron@29.x.x was the first version to include a node version later than that so an aegir update was needed to test with a more recent Electron, since we were previously using electron@27.x.x.

All good now.

@achingbrain achingbrain merged commit 0edbfe7 into libp2p:main Aug 9, 2024
25 checks passed
@achingbrain achingbrain changed the title fix: remove CustomEvent workaround. fix: remove CustomEvent polyfill Aug 9, 2024
@achingbrain achingbrain mentioned this pull request Aug 9, 2024
xendarboh added a commit to 0KnowledgeNetwork/appchain-agent that referenced this pull request Aug 26, 2024
We're on nodejs v18 until o1js/protokit upgrades.

@libp2p says: "We support LTS and Current which means v20+"

Related:
- libp2p/js-libp2p#2656
- libp2p/js-libp2p#2652

Fixes error:
TypeError: CustomEvent is not a constructor
    at TypedEventEmitter.safeDispatchEvent
(file:///.../appchain-agent/node_modules/.pnpm/@libp2p+interface@1.7.0/node_modules/@libp2p/interface/dist/src/event-target.js:53:35)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript compilation errors within @libp2p/interface/src/event-target.ts
2 participants