Skip to content

Convert to TypeScript - #27

Merged
mcmire merged 18 commits into
mainfrom
convert-to-typescript
Feb 24, 2023
Merged

Convert to TypeScript#27
mcmire merged 18 commits into
mainfrom
convert-to-typescript

Conversation

@mcmire

Copy link
Copy Markdown
Contributor

In order to align this repo with our other repos, this commit converts the existing JavaScript to TypeScript, and adds the dependencies and configuration to support this. It also adds typedoc and updates the existing GitHub workflows to publish documentation. This means we no longer need to explicitly ship type definitions as they will be generated automatically.

Finally, there were a couple of standardization-related things that were missed in previous updates, so this adds those as well. After this commit this repo should be fully compliant with the module template.

Closes#1.

@mcmire
mcmire requested a review from a team as a code ownerFebruary 22, 2023 18:49
In order to align this repo with our other repos, this commit converts
the existing JavaScript to TypeScript, and adds the dependencies and
configuration to support this. It also adds `typedoc` and updates the
existing GitHub workflows to publish documentation. This means we no
longer need to explicitly ship type definitions as they will be
generated automatically.
Finally, there were a couple of standardization-related things that were
missed in previous updates, so this adds those as well. After this
commit this repo should be fully compliant with the module template.
@mcmire
mcmireforce-pushed the convert-to-typescript branch from a253615 to 0fc1960CompareFebruary 22, 2023 18:51
Comment threaddist/createEventEmitterProxy.d.ts Outdated
Comment threadsrc/createEventEmitterProxy.ts Outdated
Comment threadsrc/createEventEmitterProxy.ts Outdated
Comment threadsrc/createEventEmitterProxy.ts Outdated
Comment threadsrc/createEventEmitterProxy.ts Outdated
Comment threadsrc/createEventEmitterProxy.ts
Comment threadsrc/createEventEmitterProxy.ts Outdated
Gudahtt
Gudahtt previously approved these changes Feb 24, 2023

@GudahttGudahtt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@mcmire

Copy link
Copy Markdown
ContributorAuthor

@Gudahtt Question: since we are importing the EventEmitter type and since that type comes from @types/node, does that package need to be a dependency of this library? Right now it's just a dev dependency, but should I promote it?

@Gudahtt

Copy link
Copy Markdown
Member

Good question. If this were any other library, I'd say yes. But I'm not sure here because it's for a built-in API. Hmm.

Comment threadsrc/createSwappableProxy.ts Outdated
@Gudahtt

Gudahtt commented Feb 24, 2023

Copy link
Copy Markdown
Member

We could create a lightweight type representing just the EventEmitter methods we rely on here. That would avoid needing to use that type directly in the createEventEmitterProxy.ts module. It'd make the types less likely to break after @types/node updates too. It should be simple enough:

type EventEmitterLike = {
eventNames: () => (string | symbol)[];
// eslint-disable-next-line @typescript-eslint/ban-types
rawListeners(eventName: string | symbol): Function[];
removeAllListeners(event?: string | symbol): EventEmitterLike;
};

It'd improve this package's compatibility with browser environments as well, letting us remove that eslint ignore comment about us using a Node.js module.

@mcmire

Copy link
Copy Markdown
ContributorAuthor

EventEmitterLike added in 6b61eac.

Comment threadsrc/types.ts
Comment threadsrc/createEventEmitterProxy.ts Outdated
Comment threadsrc/createEventEmitterProxy.ts Outdated
mcmireand others added 3 commits February 24, 2023 13:07
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>
Gudahtt
Gudahtt previously approved these changes Feb 24, 2023

@GudahttGudahtt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@mcmire

Copy link
Copy Markdown
ContributorAuthor

I need to confirm that these updates don't cause any issues with MetaMask/core#1091.

@mcmire

Copy link
Copy Markdown
ContributorAuthor

@Gudahtt Okay, MetaMask/core#1091 still passes all tests with this change, so this is good for re-approval.

@mcmire
mcmire merged commit 3f3517a into mainFeb 24, 2023
@mcmire
mcmire deleted the convert-to-typescript branch February 24, 2023 20:49
@mcmiremcmire mentioned this pull request Feb 24, 2023
Merged
Sign up for freeto 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.

Convert to TypeScript

3 participants

@mcmire@Gudahtt@Mrtenz