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

Use web standard Permissions API #3200

Merged
merged 6 commits into from Oct 27, 2019

Conversation

@kt3k
Copy link
Contributor

kt3k commented Oct 25, 2019

This PR changes the current permissions API to the web standard Permissions API.

  • Removed op_permissions, added op_query_permission
  • Added TypeError for invalid permission name and invalid urls.
  • Added a few tests for check handlings of invalid parameters.
  • For now I keep permissions object at Deno.permissions. Maybe this should be moved to navigator.permissions later.

ref:


Partially addresses #2767.

Copy link
Collaborator

ry left a comment

Nice

export class PermissionStatus {
state: PermissionState;
constructor(state: PermissionState);
}

This comment has been minimized.

Copy link
@ry

ry Oct 25, 2019

Collaborator

Could you duplicate the jsdoc comments here?

(We need to fix this situation soon...)

assertEquals(perms[perm], perm === grant);
}
testPerm({ [grant]: true }, async function envGranted(): Promise<void> {
const status0 = await Deno.permissions.query({ name: grant });

This comment has been minimized.

Copy link
@ry

ry Oct 25, 2019

Collaborator

Should we not just start using the global Permissions object?

This comment has been minimized.

Copy link
@nayeemrmn

This comment has been minimized.

Copy link
@kt3k

kt3k Oct 26, 2019

Author Contributor

OK. Let's move permissions under navigator and other classes to global.

This comment has been minimized.

Copy link
@ry

ry Oct 26, 2019

Collaborator

Maybe leave it for separate PR since it is apparently controversial

This comment has been minimized.

Copy link
@kt3k

kt3k Oct 26, 2019

Author Contributor

Sorry I misread the thread. OK. Let's keep it for now.

url?: string;
path?: string;
Comment on lines 23 to 25

This comment has been minimized.

Copy link
@nayeemrmn

nayeemrmn Oct 25, 2019

Contributor

Why not use sub types for these?

interface PermissionDescriptor {
  name: PermissionName;
}

namespace Deno {
  export interface NetPermissionDescriptor extends PermissionDescriptor {
    name: "net";
    url?: string;
  }

  // ...
}

This comment has been minimized.

Copy link
@kt3k

kt3k Oct 26, 2019

Author Contributor

Thanks! That looks nicer. I'll update.

@nayeemrmn

This comment has been minimized.

Copy link
Contributor

nayeemrmn commented Oct 25, 2019

Like I said here #2767 (comment), putting this in navigator breaks compatibility.

What about this is supposed to work outside of deno anyway? 🙄 Happened with window.location... that should be deprecated immediately IMO.

@kt3k kt3k force-pushed the kt3k:perm-api branch from 3505437 to 722af1e Oct 26, 2019
type PermissionDescriptor =
| SimplePermissionDescriptor
| PathPermissionDescriptor
| NetPermissionDescriptor;
Comment on lines 35 to 38

This comment has been minimized.

Copy link
@nayeemrmn

nayeemrmn Oct 26, 2019

Contributor

👍

Opinion: I think it would be better to give each permission its own interface so they are arbitrarily extensible e.g. #3137.

Maybe read/write have a meaningful commonality but the rest aren't related to each other.

This comment has been minimized.

Copy link
@kt3k

kt3k Oct 26, 2019

Author Contributor

Makes sense 👍 I separated descriptor types for each permission.


export class Permissions {
query(d: PermissionDescriptor): Promise<PermissionStatus>;
revoke(d: PermissionDescriptor): Promise<PermissionStatus>;

This comment has been minimized.

Copy link
@ry

ry Oct 26, 2019

Collaborator

It would be nice to have jsdoc with examples for these, since the previous API did have nice docs.

Copy link
Collaborator

ry left a comment

Looking good. There's one more documentation change needed https://deno.land/std/manual.md#inspecting-and-revoking-permissions

@kt3k kt3k force-pushed the kt3k:perm-api branch from 29752c2 to 3fbe787 Oct 27, 2019
@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Oct 27, 2019

Thank you for catching. Updated //std/manual.md.

@ry
ry approved these changes Oct 27, 2019
Copy link
Collaborator

ry left a comment

LGTM - thanks! Looking forward to having this in the next release

@ry ry merged commit efd7e78 into denoland:master Oct 27, 2019
10 checks passed
10 checks passed
test macOS-10.14
Details
test_std macOS-10.14
Details
test windows-2019
Details
test_std windows-2019
Details
test ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
test_std ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
@kt3k kt3k deleted the kt3k:perm-api branch Oct 27, 2019
@kt3k kt3k referenced this pull request Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.