Skip to content

Update TypeScript to latest stable version - #28

Merged
Gudahtt merged 1 commit into
mainfrom
update-typescript
Nov 3, 2022
Merged

Gudahtt merged 1 commit into
mainfrom
update-typescript

Conversation

@Gudahtt

@Gudahtt Gudahtt commented Oct 26, 2022

Copy link
Copy Markdown
Member

TypeScript has been updated to the latest stable version. Our standard module TypeScript configuration has been brought over from the module template as well, including a separate configuration file for production builds (as opposed to the root file used for things like tests).

A build:clean has been added as well, for consistency with the module template.

@Gudahtt

Gudahtt commented Oct 26, 2022 •

Copy link
Copy Markdown
Member Author

This depends upon #26

@Gudahtt
Gudahtt force-pushed the update-lint-configuration branch from 5770eca to f990397 Compare October 26, 2022 05:49
@Gudahtt
Gudahtt force-pushed the update-typescript branch 2 times, most recently from e310b58 to 49a8d6a Compare October 26, 2022 05:52
@Gudahtt
Gudahtt force-pushed the update-lint-configuration branch from f990397 to 98dd4b4 Compare October 26, 2022 13:48
@Gudahtt
Gudahtt force-pushed the update-typescript branch 2 times, most recently from a7ee58b to 1fd23bc Compare October 26, 2022 13:53
@Gudahtt
Gudahtt force-pushed the update-lint-configuration branch 2 times, most recently from 214c6b9 to 4325f98 Compare October 31, 2022 21:43
@Gudahtt
Gudahtt force-pushed the update-lint-configuration branch 2 times, most recently from 96f24c9 to 26b3318 Compare November 3, 2022 20:03
Base automatically changed from update-lint-configuration to main November 3, 2022 20:11
TypeScript has been updated to the latest stable version. Our standard
module TypeScript configuration has been brought over from the module
template as well, including a separate configuration file for
production builds (as opposed to the root file used for things like
tests).

A `build:clean` has been added as well, for consistency with the module
template.
@Gudahtt
Gudahtt marked this pull request as ready for review November 3, 2022 20:18
@Gudahtt
Gudahtt requested a review from a team as a code owner November 3, 2022 20:18
Comment thread src/index.ts
Comment on lines 274 to 280
export function serializeBufferForStorage(buffer: Uint8Array): string {
let result = '0x';
const len = buffer.length || buffer.byteLength;
for (let i = 0; i < len; i++) {
result += unprefixedHex(buffer[i]);
}
buffer.forEach((value) => {
result += unprefixedHex(value);
});
return result;
}

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.

This entire function could be replaced with bytesToHex from @metamask/utils.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Nice! I'm tempted to leave this inline for now until we migrate this into the monorepo, to avoid the dependency bloat problem of having incompatible util library versions (the monorepo basically solves this by making it possible for them to be kept in sync). I'll create an issue for this though, thanks for pointing it out.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

#30

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.

We could make it a peer dependency if you don't want dependency bloat? The version in the extension has the bytes functions already, a lot of functions in this library could be replaced.

@Gudahtt Gudahtt Nov 3, 2022 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The dependency bloat problem I had in mind was where we need multiple different copies due to major version updates of the utils package, which leaves us with multiple copies unless we're very diligent with updating all packages that use utils (sometimes with can take up to 4 levels of coordinated releases). Adding it as a peer dependency just moves that problem up one level. With the monorepo we are planning on using a Yarn constraint to ensure we have compatible versions across the board.

I've been holding off on replacing internal functions with utils across various different packages for this reason. The monorepo should land tomorrow or Monday though, and I am hoping that migrating a package into it will be a straightforward process. So we may be able to migrate this into the monorepo as early as next week.

@Gudahtt
Gudahtt merged commit e66f3ca into main Nov 3, 2022
@Gudahtt
Gudahtt deleted the update-typescript branch November 3, 2022 20:30
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.

2 participants