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

Argument order for locks.acquire #19

Closed
pwnall opened this issue Nov 17, 2017 · 7 comments
Closed

Argument order for locks.acquire #19

pwnall opened this issue Nov 17, 2017 · 7 comments

Comments

@pwnall
Copy link

pwnall commented Nov 17, 2017

(while it may be worse for WebIDL) I think it'd be better if the options bag preceded the callback. JS code is easier to read when functions take at most one callback, which is the last argument.

The example below is lifted from the explainer. Baseline:

  await locks.acquire('resource', async lock => {
    // Use |lock| here.
  }, {signal: controller.signal});

This proposal:

  await locks.acquire('resource', {signal: controller.signal}, async lock => {
    // Use |lock| here.
  });

So, please reconsider the argument order.

@inexorabletash
Copy link
Member

@domenic - what do you think?

We've got two conflicting precedents here - putting callbacks last, and putting options last.

@domenic
Copy link

domenic commented Nov 17, 2017

Yeah, I noticed this before. I guess I do lean toward callback-last. Preferably with an overload so you can omit the options, I guess.

@inexorabletash
Copy link
Member

Are dictionaries and callbacks distinguishable in WebIDL parlance? overloading still hurts my brain...

i.e. is this valid WebIDL ?

interface LockManager {
  Promise<any> acquire((DOMString or sequence<DOMString>) scope,
                       LockRequestCallback callback);                       
  Promise<any> acquire((DOMString or sequence<DOMString>) scope,
                       optional LockOptions options,
                       LockRequestCallback callback);
};

@domenic
Copy link

domenic commented Nov 17, 2017

Well, optional arguments before the end is not valid Web IDL, so that's a minor bug...

They are not distinguishable, but overloads allow you to make decisions based on the number of arguments passed, instead of on distinguishability. This is usually bad practice, but it seems like the best option here...

@inexorabletash
Copy link
Member

Whoops, copy/pasta. Removing optional

interface LockManager {
  Promise<any> acquire((DOMString or sequence<DOMString>) scope,
                       LockRequestCallback callback);                       
  Promise<any> acquire((DOMString or sequence<DOMString>) scope,
                       LockOptions options,
                       LockRequestCallback callback);
};

And... I guess argument count trumps distinguishability? I'll give this a whirl in Blink at least.

@inexorabletash
Copy link
Member

Thanks - yes, code is much more readable this way. Updated the readme, proto spec, idl, and added an FAQ item contrasting the two versions.

@pwnall
Copy link
Author

pwnall commented Nov 17, 2017

🎉 Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants