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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to TypeScript #1

Merged
merged 23 commits into from
Feb 13, 2020
Merged

Migrate to TypeScript #1

merged 23 commits into from
Feb 13, 2020

Conversation

whymarrh
Copy link
Contributor

@whymarrh whymarrh commented Jan 19, 2020

This PR, basically, migrates the project to use TypeScript. I've mixed in a few extra (read: too many) changes here because I'm lazy. 馃う鈥嶁檪

We've got:

  • Switch from npm to yarn
  • Add a .nvmrc file
  • Add tests
  • Convert to TypeScript
  • Add linting
  • Scope the project under the @metamask npm scope
  • Drop the events dependency (is this needed?)
  • Add CircleCI config (closes Add Circle CI config聽#2)

I'm hoping this can serve as a reference for future migrations and that these configurations (i.e. ESLint & tsconfig) can be factored out as appropriate.

Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

Just a couple of nits, otherwise LGTM.

.eslintrc.js Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
@whymarrh whymarrh marked this pull request as ready for review January 21, 2020 00:42
@whymarrh
Copy link
Contributor Author

@rekmarks I've factored out the ESLint configuration into its own project. For the purposes of this PR, we can assume that the config exists, works, and passes. (I'm just now realizing that we aren't running the build here, #2, but that's a separate PR.)

@whymarrh
Copy link
Contributor Author

I've included #2 here as well, why not

index.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
index.ts Outdated Show resolved Hide resolved
index.ts Outdated Show resolved Hide resolved
Co-Authored-By: Erik Marks <25517051+rekmarks@users.noreply.github.com>
tsconfig.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
whymarrh and others added 3 commits February 11, 2020 11:16
Co-Authored-By: Mark Stacey <markjstacey@gmail.com>
Co-Authored-By: Mark Stacey <markjstacey@gmail.com>
package.json Outdated Show resolved Hide resolved
@whymarrh
Copy link
Contributor Author

whymarrh commented Feb 12, 2020

Alrighty, I think this is good now. 100% test coverage, I've double-checked the package contents, and the CI is running.

package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
.npmignore Outdated Show resolved Hide resolved
$ tar tvf metamask-safe-event-emitter-v1.0.1.tgz
drwxr-xr-x  0 0      0           0 12 Feb 17:21 package
-rw-r--r--  0 0      0         481 21 Jan 18:04 package/README.md
-rw-r--r--  0 0      0         182 12 Feb 16:54 package/index.d.ts
-rw-r--r--  0 0      0        2115 12 Feb 16:54 package/index.js
-rw-r--r--  0 0      0        1867 12 Feb 16:54 package/index.js.map
-rw-r--r--  0 0      0         885 12 Feb 17:22 package/package.json
Co-Authored-By: Mark Stacey <markjstacey@gmail.com>
throw er; // Unhandled 'error' event
}
// At least give some kind of context to the user
const err = new Error('Unhandled error.' + (er ? ' (' + er.message + ')' : ''));
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This might be more readable using a string template?

Copy link
Member

Choose a reason for hiding this comment

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

Seconded, but non-blocking.

index.ts Show resolved Hide resolved
safeApply(handler, this, args);
} else {
const len = handler.length;
const listeners = arrayClone(handler);
Copy link
Member

Choose a reason for hiding this comment

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

Actually.... this is the only place where arrayClone is used, and it's entirely pointless? It makes a shallow clone, then discards it. The listeners array is never touched after this point - just listeners[i], which is the same as handler[i] because shallow. So arrayClone can be removed altogether.

Copy link
Member

Choose a reason for hiding this comment

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

Huh - apparently this is in the Gozala/events module as well.

Copy link
Member

Choose a reason for hiding this comment

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

It's also in node core. It was added here: nodejs/node#601
I must be missing something 馃 I don't understand why that clone is happening at all, and from skimming through the discussion they don't mention it at all. Maybe some kinda obscure micro-optimization 馃し鈥嶁檪 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah even @rekmarks' suggestions above, I assume they were all micro-optimizations in (what could be) hot code paths. I left in the len variable here for that reason. A part of me wants to clean everything up and just say "let the runtime deal with it," but... yeah. 馃し鈥嶁檪

That was 5 years ago, and runtimes have come a long way since. 馃

Copy link
Member

Choose a reason for hiding this comment

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

I was poking around on jsperf for that but couldn't find anything conclusive. The good news is, it's definitely fine, and there's no reason to block on it. I don't see any reason not to do [ ...handler ], but 馃し鈥嶁檪

Copy link
Member

Choose a reason for hiding this comment

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

It turns out the clone is necessary 馃う鈥嶁檪 . Kinda obvious in retrospect - the clone happens here because the event handlers might subscribe new handlers, or unsubscribe them, thus mutating the handlers array.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

LGTM.

safeApply(handler, this, args);
} else {
const len = handler.length;
const listeners = arrayClone(handler);
Copy link
Member

Choose a reason for hiding this comment

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

I was poking around on jsperf for that but couldn't find anything conclusive. The good news is, it's definitely fine, and there's no reason to block on it. I don't see any reason not to do [ ...handler ], but 馃し鈥嶁檪

throw er; // Unhandled 'error' event
}
// At least give some kind of context to the user
const err = new Error('Unhandled error.' + (er ? ' (' + er.message + ')' : ''));
Copy link
Member

Choose a reason for hiding this comment

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

Seconded, but non-blocking.

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.

Add Circle CI config
3 participants