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
Refactor browserContext.grantPermissions #1075
Conversation
b8e6390
to
08e7912
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
08e7912
to
d5c55a6
Compare
9d81296
to
3f1f503
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
3f1f503
to
f7f3714
Compare
This change aligns the options parsing with the other APIs.
Instead of panicking in grantPermissions we should return an error to prevent unnecessary panics.
This moves the goja value parsing into the mapping layer so that we're removing goja from the browserContext business logic, which makes the code a bit more maintainable.
This change makes it explicit when a permission is used that the module doesn't recognise, allowing the user to fix the mistake quickly instead of having to debug the issue if the module just hid the invalid permissions.
Co-authored-by: İnanç Gümüş <inanc.gumus@grafana.com>
f7f3714
to
50992a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
What?
This refactors the
browserContext.grantPermissions
implementation.Why?
This helps bring it inline with current ways of implementing the APIs:
error
instead of panicking.Checklist
Related PR(s)/Issue(s)
Prerequisite to working on #443