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

Refactoring from fetch to XMLHttpRequest should not break consumers of my library #22

Open
bakkot opened this issue Feb 1, 2023 · 5 comments

Comments

@bakkot
Copy link

bakkot commented Feb 1, 2023

This was discussed at some length on Matrix starting around here, but I want to capture it on this tracker so it doesn't get lost.

I think that it is not reasonable to ask every user, even those who are not using async contexts themselves, to know that Promise.prototype.then is a fundamentally different kind of thing than other mechanisms of scheduling callbacks, like XMLHttpRequest.prototype.onload.

That is, if I am not myself using async contexts, I absolutely should not have to know that a normally transparent refactoring like going from

function makeRequestThen(callback) {
  fetch(url).then(x => x.text()).then(result => callback(null, result), err => callback(err))
}

to

function makeRequestThen(callback) {
  const req = new XMLHttpRequest();
  req.addEventListener('load', () => callback(null, req.responseText));
  req.addEventListener('error', err => callback(err));
  req.open('GET', url);
  req.send();
}

is in fact a breaking change, because the former preserves the context for callback and the latter does not.

So either Promise.prototype.then should not automatically wrap its argument in the current context, or we should require of hosts that any mechanism for scheduling callbacks which might run in a later tick or turn should automatically wrap their argument in the current context.

(Or I'm misunderstanding how this whole proposal works, which is still possible.)

@littledan
Copy link
Member

I believe the Web Platform should perform a wrap in the latter case as well. We'll need to develop a much more detailed model for this before Stage 3.

@benjamingr
Copy link

Note in the particular case: fetch(url).then(x => x.text()).then(result => callback(null, result), err => callback(err)) has different semantics than the onload/onerror above. Namely if callback throws.

or we should require of hosts that any mechanism for scheduling callbacks which might run in a later tick or turn should automatically wrap their argument in the current context.

That sounds very reasonable.

@bakkot
Copy link
Author

bakkot commented Feb 4, 2023

has different semantics than the onload/onerror above. Namely if callback throws

Yeah, it was just meant to be an off-the-cuff example; I didn't try to make it actually equivalent. That said, what's the difference you're pointing at? Only the fetch one will trigger the unhandled rejection handler, as opposed to the onerror handler, I guess; is there something else I'm missing?

@benjamingr
Copy link

Only the fetch one will trigger the unhandled rejection handler, as opposed to the onerror handler, I guess; is there something else I'm missing?

Basically you got the important one:

  • The fetch throwing rejects a promise so it will trigger an unhandled rejection if "callback" throws whereas the onload will trigger a global error event (in browsers).

There are a few others like:

  • If you returns a rejected promise from the fetch example where the error came from might surprise you, but in general "return value".
  • The this value being the XHR object vs. undefined.

But the important point was that we need a mechanism for hosts to specify what context a continuation runs in.

@jridgewell
Copy link
Member

Related: #19

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

No branches or pull requests

4 participants