-
-
Notifications
You must be signed in to change notification settings - Fork 48
Typescript migration + standardization #140
Conversation
27eedd6
to
174c96a
Compare
174c96a
to
94d7ca5
Compare
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.
Nice work! I didn't go through everything yet, but I left some small comments
411af32
to
60f1652
Compare
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.
I took a more detailed look into it, I left some suggestions around :)
f483441
to
37df0ab
Compare
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.
Good job on migrating this library. There were some TypeScript-related things I noticed but nothing major. A couple of things I wanted to highlight:
- It's important not to bump the version just yet to avoid a premature release (I've marked this as "requested changes" for this case in particular just so you notice)
- It seems that this PR does more than just migrate to TypeScript — it also brings in all of the GitHub workflows, VSCode settings, bumps the Node version, etc. — basically a bunch of standardization stuff — so it would probably be good to retitle the PR and/or update the PR description to encompass these changes so no one is caught off guard.
New dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No new dependency issues detected in pull request Bot CommandsTo ignore an alert, reply with a comment starting with Ignoring: Pull request alert summary
📊 Modified Dependency Overview:
🚮 Removed packages: @metamask/[email protected], [email protected], [email protected], [email protected] |
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.
Looks like there are a few more outstanding comments here (check hidden conversations) but this is looking good so far.
bdfb5da
to
64e50bf
Compare
Dismissing since the version change has been reverted.
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 for patiently working through my feedback. Nice work!
@SocketSecurity ignore-all |
Node andnpm packages