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

Conversation

@gantunesr
Copy link
Member

@gantunesr gantunesr commented Apr 19, 2023

There is a false positive in the current CI caused by using TypeScript v.5.0.2 and ts-jest v29.0.3. This development updates it to the required version and ignores the error in the meanwhile.

Screenshot 2023-04-19 at 13 45 21

The PR MetaMask/utils#99 should solve the root cause of the TS error.

@gantunesr gantunesr marked this pull request as ready for review April 19, 2023 18:18
@gantunesr gantunesr requested a review from a team as a code owner April 19, 2023 18:18
@gantunesr gantunesr merged commit 0fad94e into main Apr 19, 2023
@gantunesr gantunesr deleted the fix/ts-jest-version branch April 19, 2023 20:20
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! one suggestion though

Comment on lines +1039 to +1041
// @ts-expect-error The method 'init' is not part of the current Keyring type
if (keyring.init) {
// @ts-expect-error The method 'init' is not part of the current Keyring type
Copy link
Member

Choose a reason for hiding this comment

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

Nit: We can check for this function in a way that TypeScript understands, rather than suppress the error:

Suggested change
// @ts-expect-error The method 'init' is not part of the current Keyring type
if (keyring.init) {
// @ts-expect-error The method 'init' is not part of the current Keyring type
if (hasProperty(keyring, 'init') && typeof keyring.init === 'function') {

The hasProperty function used here would have to be imported, but it's in @metamask/utils which is already a dependency.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll open another PR to include your suggestion @Gudahtt

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants