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

Remove XMLHttpRequest from ServiceWorkerGlobalScope #19

Closed
foolip opened this issue May 26, 2015 · 18 comments
Closed

Remove XMLHttpRequest from ServiceWorkerGlobalScope #19

foolip opened this issue May 26, 2015 · 18 comments

Comments

@foolip
Copy link
Member

foolip commented May 26, 2015

I noticed a discrepancy between the spec IDL and Blink, that comes down to this bug:
https://code.google.com/p/chromium/issues/detail?id=395931

If this makes sense, I suggest replacing Exposed=(Window,Worker) with Exposed=(Window,DedicatedWorker,SharedWorker) in the spec too.

CC @coonsta and @jakearchibald

@annevk
Copy link
Member

annevk commented May 30, 2015

So the idea is to not expose XMLHttpRequest in new contexts in order to encourage usage of fetch()... Seems okay I guess.

@tyoshino
Copy link
Member

tyoshino commented Jun 1, 2015

Sounds good

@foolip
Copy link
Member Author

foolip commented Jun 1, 2015

I was half expecting push back on this. Won't it be annoying that you can't reuse some existing library code because it uses XHR? From https://code.google.com/p/chromium/issues/detail?id=395931 it sounds like part of the motivation was to get rid of the sync mode, but is that a bigger problem for service workers than other kinds of workers?

@tyoshino
Copy link
Member

tyoshino commented Jun 8, 2015

The pain in implementation would be small if we're to keep only async XHR, I guess. Not sure if there're works such that:

  • it makes sense to get the works done in SW, and
  • (part of) the works don't require the Fetch API's functionality, and
  • complicated so that use of library makes sense

@annevk
Copy link
Member

annevk commented Jun 9, 2015

I was actually expecting us to simply forbid synchronous XMLHttpRequest in SW, but leave it working otherwise. However, since fetch() shipped relatively quickly I guess I do not really care either way. If there is some kind of win in not exposing it (or maybe there is one down the line) I would be cool with that.

What about ProgressEvent?

@foolip
Copy link
Member Author

foolip commented Jun 9, 2015

Actually ProgressEvent is exposed only on Window in Blink, but I believe that's an oversight. I think the spec should expose all the interfaces in the same contexts and Blink should change.

To me, throwing InvalidAccessError when sync XHR is attempted in any non-whitelisted context sounds pretty good. Which kinds of workers already depend on sync XHR I don't know, but we could measure it.

@coonsta and @jakearchibald, thoughts?

@dominiccooney
Copy link

I'm sorry that we didn't file a spec bug and link to it from that patch; we should always do that. @slightlyoff is a better person to ask about exposing XHR; @kinu is a better person to ask for implementer feedback from Chromium/Blink.

@annevk
Copy link
Member

annevk commented Jul 16, 2015

Ping!

@foolip
Copy link
Member Author

foolip commented Aug 14, 2015

Ping again @coonsta, what do you think we should do here?

@kinu
Copy link

kinu commented Aug 14, 2015

Only shipping async XHR in SW makes sense to me and I don't feel it's too difficult to implement in Chromium/Blink. I'm not really sure if exposing XHR is still awaited since SW was shipped without XHR sometime ago and now we have fetch, but having it may make it easier to use JS libraries in SW I guess.
/cc @mattto

@mfalken
Copy link

mfalken commented Aug 19, 2015

I haven't heard users complain about lack of XHR in Chrome's SW implementation yet, so maybe there's no need to add async XHR now. We can always add it later if needed.

@foolip
Copy link
Member Author

foolip commented Aug 19, 2015

OK, so I guess let's change the spec instead.

@annevk annevk closed this as completed in 9d0a9bb Aug 19, 2015
@annevk
Copy link
Member

annevk commented Aug 19, 2015

Thank you folks!

MXEBot pushed a commit to mirror/chromium that referenced this issue Jan 16, 2016
whatwg/xhr#19

BUG=395931
[email protected]

Review URL: https://codereview.chromium.org/1310543002

git-svn-id: svn://svn.chromium.org/blink/trunk@201597 bbb929c8-8fbe-4397-9dbb-9b2b20218538
@vandys
Copy link

vandys commented Mar 28, 2017

With cancellation and timeouts (still) missing from fetch, would it be appropriate to revisit
access to XMLHttpRequest in Service Workers?

@wanderview
Copy link
Member

With cancellation and timeouts (still) missing from fetch, would it be appropriate to revisit
access to XMLHttpRequest in Service Workers?

Cancellation is making progress. We have experimental support for it in FF55 if you set dom.fetchController.enabled to true.

https://twitter.com/wanderview/status/844924163634679808

@CyrilCommando
Copy link

CyrilCommando commented Sep 11, 2022

Let me get this straight - The option to use XHR in service workers was removed because one guy thinks it's "redundant", and is partial to fetch?

Options be damned!

@annevk
Copy link
Member

annevk commented Sep 12, 2022

Well, we also want folks to move to fetch(). That started in 2015 and is especially true today.

@vandys
Copy link

vandys commented Sep 12, 2022

Well, we also want folks to move to fetch(). That started in 2015 and is especially true today.

fetch() eventually added an abort, but you still have to cobble together your own timeout. And the abort is quite a bit clunkier than what it takes for an XHR. I hope in the future you'll make sure your API's are a superset (and preferably cleaner) before forcing the sunset of old API's.

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

No branches or pull requests

9 participants