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

Fix permission-controller lint violations #4537

Merged
merged 2 commits into from
Jul 18, 2024

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Jul 17, 2024

Explanation

There are some permission-controller tests which attempt to push an async middleware function onto an engine. Although this works in practice, @typescript-eslint balks at this, because the type for JsonRpcEngine.push says it takes a JsonRpcMiddleware, and a JsonRpcMiddleware has a return type of void, not Promise<void>. We could change that return type to void | Promise<void> but that would cause other changes we may not want to make. So this commit merely ignores the lint violations. This should be safe because JsonRpcEngine doesn't await middleware anyway.

Interestingly, the lint violations caused by this discrepancy only appear locally and not on CI. I am not sure why this is.

References

Also see: #4521

Changelog

(No consumer-facing changes to log)

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

There are some `permission-controller` tests which attempt to push an
async middleware function onto an engine. Although this works in
practice, `@typescript-eslint` balks at this, because the type for
`JsonRpcEngine.push` says it takes a `JsonRpcMiddleware`, and a
`JsonRpcMiddleware` has a return type of `void`, not `Promise<void>`. We
could change that return type to `void | Promise<void>` but that would
cause other changes we may not want to make. So this commit merely
ignores the lint violations.

Interestingly, the lint violations caused by this discrepancy only
appear locally and not on CI. I am not sure why this is.
@mcmire mcmire requested a review from a team as a code owner July 17, 2024 22:19
@mcmire mcmire requested review from rekmarks and a team July 18, 2024 17:13
@mcmire
Copy link
Contributor Author

mcmire commented Jul 18, 2024

@rekmarks You may be interested in this PR since you made some changes to the permission controller recently. Are you able to replicate these lint violations?

Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

@mcmire LGTM, but beware that something fishy is going on: #4540

I can't observe the failures you're seeing. I thought "I'm just having some local issue, I'll ignore it", but it seems there's actually something wrong.

@mcmire mcmire merged commit ed7a9db into main Jul 18, 2024
116 checks passed
@mcmire mcmire deleted the fix-permission-controller-lint-violations branch July 18, 2024 22:18
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