Skip to content
This repository has been archived by the owner on Oct 7, 2024. It is now read-only.

Typescript migration + standardization #140

Merged
merged 14 commits into from
Mar 28, 2023
Merged

Conversation

HowardBraham
Copy link
Contributor

@HowardBraham HowardBraham commented Mar 9, 2023

@HowardBraham HowardBraham added the team-accounts This should be handled by the Accounts Team label Mar 9, 2023
@HowardBraham HowardBraham self-assigned this Mar 9, 2023
Copy link
Member

@mikesposito mikesposito left a 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

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
scripts/prepack.sh Outdated Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@gantunesr gantunesr marked this pull request as ready for review March 15, 2023 20:04
@gantunesr gantunesr requested a review from a team as a code owner March 15, 2023 20:04
src/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@mikesposito mikesposito left a 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 :)

src/index.ts Outdated Show resolved Hide resolved
src/index.test.ts Outdated Show resolved Hide resolved
src/index.test.ts Outdated Show resolved Hide resolved
src/index.test.ts Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.vscode/extensions.json Show resolved Hide resolved
mcmire
mcmire previously requested changes Mar 21, 2023
Copy link

@mcmire mcmire left a 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.

.eslintrc.js Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/sample.ts Show resolved Hide resolved
src/simple-keyring.test.ts Outdated Show resolved Hide resolved
src/simple-keyring.test.ts Outdated Show resolved Hide resolved
src/simple-keyring.test.ts Outdated Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
@socket-security
Copy link

socket-security bot commented Mar 25, 2023

New dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] bar@* or ignore all packages with @SocketSecurity ignore-all

Ignoring: [email protected], [email protected]

Pull request alert summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package Capability Access +/- Transitive Count Publisher
@types/[email protected] None +1 types
@metamask/[email protected] None +3 metamaskbot
[email protected] None +28 holgerd77
@types/[email protected] None +0 types
[email protected] filesystem +0 typescript-bot
@metamask/[email protected] None +5 metamaskbot
[email protected] filesystem +30 rumpl
[email protected] filesystem, shell, environment +16 cspotcode
[email protected] filesystem, environment +79 kul
@typescript-eslint/[email protected] None +1 jameshenry
@typescript-eslint/[email protected] None +4 jameshenry
⬆️ Updated Package Version Diff Capability Access +/- Transitive Count Publisher
[email protected] 27.5.1...29.5.0 None +75/-104 simenb
@types/[email protected] 26.0.24...29.5.0 None +13/-8 types

🚮 Removed packages: @metamask/[email protected], [email protected], [email protected], [email protected]

Copy link

@mcmire mcmire left a 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.

@HowardBraham HowardBraham force-pushed the typescript-migration branch 2 times, most recently from bdfb5da to 64e50bf Compare March 28, 2023 06:56
@mcmire mcmire dismissed their stale review March 28, 2023 16:58

Dismissing since the version change has been reverted.

src/simple-keyring.ts Outdated Show resolved Hide resolved
Copy link

@mcmire mcmire left a 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!

@mcmire
Copy link

mcmire commented Mar 28, 2023

@SocketSecurity ignore-all

@HowardBraham HowardBraham changed the title Typescript migration Typescript migration + standardization Mar 28, 2023
@HowardBraham HowardBraham merged commit 4abfbe9 into main Mar 28, 2023
@HowardBraham HowardBraham deleted the typescript-migration branch March 28, 2023 20:05
@Gudahtt Gudahtt mentioned this pull request Sep 18, 2023
legobeat added a commit to legobeat/eth-simple-keyring that referenced this pull request Oct 20, 2023
legobeat added a commit that referenced this pull request Oct 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
team-accounts This should be handled by the Accounts Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants