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

chore: Small cleanup to vm._emit #3396

Merged
merged 2 commits into from
May 7, 2024
Merged

chore: Small cleanup to vm._emit #3396

merged 2 commits into from
May 7, 2024

Conversation

roninjin10
Copy link
Collaborator

  • Move it to an instance method. There shouldn't be any issues with it being an instance method since the underlying event emitter is still per object.
  • Make it more typesafe by taking keyof Events rather than string
  • TODO meant to do this pr to master not vitest-update

@acolytec3 acolytec3 changed the base branch from vitest-update to master May 3, 2024 19:01
@acolytec3
Copy link
Contributor

I switched it over to master and looks like it's accidentally grabbed all of the commits from my PR. Maybe just cherry-pick to a new PR?

@acolytec3 acolytec3 changed the base branch from master to vitest-update May 3, 2024 19:02
@roninjin10
Copy link
Collaborator Author

Done! This pr is in draft only because I haven't ran any tests or typecheckers yet and my type change might break some downstream usages of the function since it's stricter now.

If pr is green I'll move it out of draft.
If pr is not green I'll fix issues then move it out of draft

packages/vm/src/vm.ts Outdated Show resolved Hide resolved
packages/vm/src/vm.ts Outdated Show resolved Hide resolved
packages/vm/src/vm.ts Outdated Show resolved Hide resolved
@roninjin10 roninjin10 changed the base branch from vitest-update to master May 7, 2024 13:54
@roninjin10 roninjin10 marked this pull request as ready for review May 7, 2024 14:00
- Move it to an instance method. There shouldn't be any issues with it being an instance method since the underlying event emitter is still per object.
- Make it more typesafe by taking keyof Events rather than string

Syntax error

fix broken build

run linter
Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants