background-sync icon indicating copy to clipboard operation
background-sync copied to clipboard

hasPermission() implies boolean

Open annevk opened this issue 10 years ago • 24 comments

I think we should just go with permission() similar to the Notifications API. (The reason it cannot be a property is because we want the result to be asynchronous and the result can change over time.)

annevk avatar Feb 11 '15 14:02 annevk

Or permissionStatus(). That said, I'm hoping to remove this call from the API and rely on https://w3c.github.io/permissions/ instead.

jkarlin avatar Feb 11 '15 14:02 jkarlin

I think we should remove it. The Push API had a similar surface but it has been removed (indeed in favor of the Permissions API).

https://groups.google.com/a/chromium.org/d/msg/blink-dev/UdGlL9PtBLo/0mPKlCXyYT8J

Pull request coming.

KenjiBaheux avatar Feb 19 '15 06:02 KenjiBaheux

+1. Thanks for fixing it Kenji.

See commit a61f75d3565ae61c94fc5f606b4a00dd5c2ba222

jkarlin avatar Feb 19 '15 12:02 jkarlin

Reopening.

hasPermission() or similar APIs should be part of every API. That they will someday be covered by a permission API doesn't remove the need for them now, and elongates the critical path to shipping this feature usefully, which is an anti-pattern.

slightlyoff avatar Feb 23 '15 22:02 slightlyoff

I imagined that permissions API would be low risk but you are right. Do I need to send a new pull request or can we just revert it?

On Tue, Feb 24, 2015, 7:01 AM Alex Russell [email protected] wrote:

Reopening.

hasPermission() or similar APIs should be part of every API. That they will someday be covered by a permission API doesn't remove the need for them now, and elongates the critical path to shipping this feature usefully, which is an anti-pattern.

— Reply to this email directly or view it on GitHub https://github.com/slightlyoff/BackgroundSync/issues/39#issuecomment-75646302 .

KenjiBaheux avatar Feb 24 '15 00:02 KenjiBaheux

If we want to return a string we should just name it permission() in line with the Notifications API. If a boolean is fine hasPermission() works.

annevk avatar Feb 24 '15 07:02 annevk

I've put hasPermission back. I agree that hasPermission sounds like a bool but let's stick with hasPermission since that's what Push has.

jkarlin avatar Mar 02 '15 18:03 jkarlin

I might add that hasPermission for Push is currently not without controversy, so maybe Push is not a good example in this case.

mvano avatar Mar 02 '15 18:03 mvano

Then I vote for getPermissionStatus() as it's similar to getRegistration()

jkarlin avatar Mar 02 '15 18:03 jkarlin

I wish we made it a little less Java-esque and used shorter names (we frequently get feedback that names are too long). E.g. get(id), getAll(), and permission(), but I guess I'm okay either way...

annevk avatar Mar 03 '15 08:03 annevk

@annevk : I'm ok with permission() if it allows us to also add it to the Notification API without blocking on the Permissions API. That said, the naming of permission() makes it also look like you can request the permission with the function, which isn't the contract. So I have a mild preference for hasPermission()

slightlyoff avatar Mar 20 '15 20:03 slightlyoff

Could you explain why you don't think that hasPermission() implies it returns a boolean?

annevk avatar Mar 25 '15 17:03 annevk

Agree that hasPermission() suggests boolean. permission() or permissionStatus() seem fine to me.

jakearchibald avatar Mar 27 '15 13:03 jakearchibald

Preference for permissionState() because:

KenjiBaheux avatar Mar 31 '15 06:03 KenjiBaheux

I prefer getPermission. While I recognize one could mistake it for performing a request from the user, that meaning is not strongly implied, usually get implies a read-only action without such side effects. If it was to request permission from the user, surely it would be called something like requestPermission.

I concede that getPermissionState is more clear, but this alternative seems prohibitively verbose.

I could be ok with permission() or permissionState() but I'd prefer if the method name is a verb.

Can we try to resolve this issue? I'd like the Push API and BackgroundSync API to use the same name, the issue for the Push API is here: https://github.com/w3c/push-api/issues/136

mvano avatar Apr 20 '15 13:04 mvano

I think insisting on method names being verbs has led to some of the worst, Java-ish APIs on the web platform. More modern APIs have eschewed this restriction, and been much better for it.

permissionState() seems nice.

domenic avatar Apr 20 '15 14:04 domenic

@domenic : do you have any recent examples where not insisting on verbs worked out well? Are there any articles or posts that pull that discussion together?

mvano avatar Apr 20 '15 14:04 mvano

Fetch is a great example (.json(), .text(), etc.). DOM is another (.before(), .after(), etc.). ES2015 is another (.entries(), .keys(), .values(), [Symbol.iterator](), etc.). I could give more if you'd like.

domenic avatar Apr 20 '15 14:04 domenic

@domenic : I think those are good examples, thanks. I'd prefer permissionState() then.

mvano avatar Apr 20 '15 14:04 mvano

Why not just permission() then, similar to the Notifications API? nudge nudge

annevk avatar Apr 20 '15 15:04 annevk

@annevk : Wouldn't it be confusing to define something different with the same name? What if the Notifications API wanted to expose a method like this?

mvano avatar Apr 20 '15 15:04 mvano

If we can ever manage to steer people away from the synchronous way the Notifications API exposes this it's probably because everyone is using the Permissions API and all of this is no longer relevant?

I mostly care about using shorter names where the next is already sufficient.

annevk avatar Apr 21 '15 16:04 annevk

The notifications API screwed this up by making .permission a synchronous getter. Using it as a pun only invites confusion. This will need a different name. permissionState() or status() seems fine, with status() creating other potential issues.

slightlyoff avatar Apr 21 '15 18:04 slightlyoff

For the Push API, we went for permissionState() as that seems to have most traction overall: https://github.com/w3c/push-api/issues/136

mvano avatar Apr 22 '15 17:04 mvano