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

Maybe allow respondWith to take a promise-returning function? #40

Closed
domenic opened this issue Feb 17, 2021 · 15 comments
Closed

Maybe allow respondWith to take a promise-returning function? #40

domenic opened this issue Feb 17, 2021 · 15 comments
Labels
surface api Minor tweaks or additions to the API surface to make it nicer (without changing the model)

Comments

@domenic
Copy link
Collaborator

domenic commented Feb 17, 2021

A lot of code examples I'm writing are of the sort

appHistory.addEventListener("navigate", e => {
  e.respondWith((async () => {
    // Do actual async stuff here
  })());
});

It'd be nicer if this could be written as

appHistory.addEventListener("navigate", e => {
  e.respondWith(async () => {
    // Do actual async stuff here
  });
});

with the exact same semantics.

On the other hand, we haven't done this for other promise-accepting APIs on the platform (e.g. service worker's waitUntil() and respondWith()). Probably it's a bit too magic. (I'd be interested to hear if there were historical deliberations about this; /cc @jakearchibald @annevk.)

The real solution here is async do expressions in the language (/cc @bakkot):

appHistory.addEventListener("navigate", e => {
  e.respondWith(do async {
    // Do actual async stuff here
  });
});

but do expressions seem to be one of those features which is really bad at getting through TC39's consensus model so I'm not sure it's a good idea to wait for them.

@domenic domenic added the surface api Minor tweaks or additions to the API surface to make it nicer (without changing the model) label Feb 17, 2021
@bakkot
Copy link

bakkot commented Feb 17, 2021

but do expressions seem to be one of those features which is really bad at getting through TC39's consensus model

I am hopeful that they'll advance at the next meeting (March 9), time permitting, or if time does not permit at the following one (April 19, the dates are weird because we tweaked the schedule this year). I have a concrete change to make (allowing return and break) which I believe will address the one expressed blocking concern, and from an informal survey of delegates there was more support for that change than there was opposition to it (and no one who said they'd block).

If do fails again at that meeting I'll probably try to advance async do on its own, since it avoids some of the thornier parts of do: in particular it cannot possibly return the outer function.

@tbondwilkinson
Copy link
Contributor

I know it's "old" but there's always new Promise()

appHistory.addEventListener("navigate", e => {
  e.respondWith(new Promise((resolve) => {
     
  }));
});

And truthfully I don't know how much people will want to be using inline things here. They'll probably call out to some method.

appHistory.addEventListener("navigate", e => {
  e.respondWith(router.resondWith(e));
});

@domenic
Copy link
Collaborator Author

domenic commented Feb 17, 2021

Sorry, I should have expanded my example; in most cases I want to use await inside, which doesn't work with new Promise().

Your point about people calling out to a method is a good one, and makes me more comfortable doing nothing here except waiting for async do. That is, this issue is probably more painful when writing explainers and examples than it would be for actual users.

@jakearchibald
Copy link

I encounter the same thing in service workers, and would happily make functions work here if no one objects. I have a vague memory of suggesting it before and folks weren't keen.

@annevk
Copy link

annevk commented Feb 18, 2021

Oh, if you can find that discussion @jakearchibald that would be great. Reading OP again do async does seem a little better. It's not like we accept non-async functions into methods that need a plain value either.

@jakearchibald
Copy link

Hm, I can't find a reference to it, maybe it was in-person or I'm making it up.

@samthor
Copy link
Contributor

samthor commented Aug 19, 2021

Could we bring this back now that we have transitionWith()? I'd happily have that accept a function that is called.

@domenic
Copy link
Collaborator Author

domenic commented Aug 19, 2021

Yeah, it seems like TC39 is not moving very fast on this; I'm inclined to make this change across the web ecosystem for all promise-accepting arguments.

@samthor
Copy link
Contributor

samthor commented Aug 19, 2021

Sure, and I would be a fan of it for respondWith() too, but here at least there's a brand new method that can have new semantics from the get go.

@bathos
Copy link

bathos commented Aug 19, 2021

Is the idea that it would take either a promise or function?

I can't find any Promise<T> usage in any spec where T is or includes a callback function or callback interface type. That makes it seem plausible that "if it's callable, it gets called" could be a first-class aspect of Web IDL's Promise<T> conversion rules. An explicit "T can't be callable" restriction would be needed to make that sound.

Would that be weird? It does seem like a platform API consuming or returning Promise<something callable> is very unlikely to be useful or preferable over alternative patterns.

The alternative seemingly would be to permit Promise<T> in unions, but that seems a lot more fraught with potential negative consequences.

@bathos
Copy link

bathos commented Aug 19, 2021

Ah, damn: customElements.whenDefined was changed to return Promise<CustomElementConstructor> recently (or maybe it was always like this actually? I may be thinking of define() itself changing), so nevermind re: "can't find any".

@jakearchibald
Copy link

@domenic

Yeah, it seems like TC39 is not moving very fast on this; I'm inclined to make this change across the web ecosystem for all promise-accepting arguments.

+1 for this.

@domenic
Copy link
Collaborator Author

domenic commented Aug 19, 2021

@bathos we would only apply it on the input conversion (i.e. ES to Web IDL), so that example is not an issue.

@annevk
Copy link

annevk commented Sep 6, 2021

I think I would prefer waiting a little longer with this, but either way, can we close this issue and open a follow-up against Web IDL? I hope there is agreement that it shouldn't be done for a single API.

@domenic
Copy link
Collaborator Author

domenic commented Sep 7, 2021

Sure: whatwg/webidl#1020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
surface api Minor tweaks or additions to the API surface to make it nicer (without changing the model)
Projects
None yet
Development

No branches or pull requests

7 participants