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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump typescript from ~5.0.4 to ~5.1.6 #4576

Merged
merged 4 commits into from
Aug 2, 2024
Merged

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Jul 31, 2024

References

Changelog

### Changed

- Upgrade TypeScript version to `~5.1.6` ([#4576](https://github.com/MetaMask/core/pull/4576))

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

Copy link

socket-security bot commented Jul 31, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/typescript@5.1.6 None 0 40.1 MB typescript-bot

🚮 Removed packages: npm/typescript@5.0.4

View full report↗︎

@MajorLift MajorLift changed the title Bump typescript from ~5.0.4 to ~5.1.6 Bump typescript from ~5.0.4 to ~5.1.6 Jul 31, 2024
@MajorLift MajorLift force-pushed the bump-typescript-to-5.1 branch 2 times, most recently from 400fcca to 703d3c4 Compare July 31, 2024 22:38
@MajorLift MajorLift marked this pull request as ready for review August 1, 2024 15:04
@MajorLift MajorLift requested review from a team as code owners August 1, 2024 15:04
@MajorLift MajorLift requested a review from a team August 1, 2024 15:05
Comment on lines +167 to +175
const implementation = this.implementations[type].method as (
...args: Parameters<RateLimitedApis[ApiType]['method']>
) => ReturnType<RateLimitedApis[ApiType]['method']>;

if (!implementation) {
throw new Error('Invalid api type');
}

return implementation(...args) as ReturnType<
RateLimitedApis[ApiType]['method']
>;
return implementation(...args);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MetaMask/snaps-devs This change fixes the following regression:

ERROR: src/RateLimitController.ts:173:27 - error TS2345: Argument of type 'Parameters<RateLimitedApis[ApiType]["method"]>' is not assignable to parameter of type 'never'.
  Type 'unknown[]' is not assignable to type 'never'.
    Type 'Parameters<RateLimitedApis[keyof RateLimitedApis]["method"]>' is not assignable to type 'never'.
      Type 'unknown[]' is not assignable to type 'never'.
        Type 'Parameters<RateLimitedApis[string | number | symbol]["method"]>' is not assignable to type 'never'.
          Type 'unknown[]' is not assignable to type 'never'.
            Type 'Parameters<RateLimitedApis[string]["method"]> | Parameters<RateLimitedApis[number]["method"]> | Parameters<RateLimitedApis[symbol]["method"]>' is not assignable to type 'never'.
              Type 'Parameters<RateLimitedApis[string]["method"]>' is not assignable to type 'never'.
                Type 'unknown[]' is not assignable to type 'never'.
                  Type 'never[]' is not assignable to type 'never'.

173     return implementation(...args) as ReturnType<

Both before and after the change, the as assertion narrows the type of implementation, which is currently typed to the universal supertype (i.e. type constraint) of all RateLimitedApi method types (i.e. ActionConstraint) because there's no ApiType information to narrow it.

The only difference is that the type assertion now narrows the parameters (...args) of implementation, in addition to its return type.

@MajorLift MajorLift merged commit d4702a2 into main Aug 2, 2024
116 checks passed
@MajorLift MajorLift deleted the bump-typescript-to-5.1 branch August 2, 2024 16:36
@Gudahtt Gudahtt mentioned this pull request Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to TypeScript v5.1
4 participants