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

BREAKING(permissions): deprecate permissions module #3567

Merged
merged 2 commits into from Aug 25, 2023

Conversation

lino-levan
Copy link
Contributor

Closes #3557

@lino-levan lino-levan requested a review from kt3k as a code owner August 23, 2023 01:16
permissions/mod.ts Outdated Show resolved Hide resolved
* prompt for it. The function resolves with all of the granted permissions. */
* prompt for it. The function resolves with all of the granted permissions.
*
* @deprecated (will be removed in 1.0.0) Use the Deno permission API instead
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the deprecation notice present when using every overload or just this one? If just the one, I'd have a sweep through the codebase and other current PRs to make the appropriate adjustments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this specific case, it's fine since we've deprecated ever individual overload (outside of the base one, which seems inaccessible? Could I get you to check that?), but I'd definitely make sure we've properly deprecated every overload.

Speaking of checking the inaccessibility of the base thing. These two work:

grant({name: "net"}, {name: "net"})
grant([{name: "net"}, {name: "net"}])

This doesn't:

grant([{name: "net"}], {name: "net"})

Why? Am I reading this type wrong?

Copy link
Collaborator

@iuioiua iuioiua Aug 23, 2023

Choose a reason for hiding this comment

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

Ah, whoops didn't see that each overload had the declaration. Either way, I'd check elsewhere if an overload needs to include the declaration.

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@kt3k kt3k merged commit d36beb2 into denoland:main Aug 25, 2023
9 checks passed
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.

Deprecate permissions module
3 participants