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

feat(service-worker): Support ignoring specific URLs (e.g. for AJAX progress listener, video streaming, range requests, Firebase uploads) #21191

Closed
MrCroft opened this issue Dec 27, 2017 · 42 comments

Comments

Projects
None yet
@MrCroft
Copy link

commented Dec 27, 2017

I'm submitting a...

[x] Feature request

Current behavior

ServiceWorker breaks AJAX uploads progress reporting.

Expected behavior

AJAX upload progress listener should work.

What is the motivation / use case for changing the behavior?

Progress is needed when handling uploads.
There is an open issue here and it is on this w3c roadmap
Until then, as far as I could read, it would be possible to make this work by checking the event.request.url at the very top of the SW script and simply return if it matches certain urls that would be specified in ngsw-config.json

Environment

Angular version: 5.1.2
@doronsever

This comment has been minimized.

Copy link

commented Jan 21, 2018

We have the same problem either.
For now, we added an ugly spinner instead of a progress bar but our clients aren't happy with it.

It would be best if this topic can be priorities somehow

@ngbot ngbot bot added this to the Backlog milestone Jan 23, 2018

@MrCroft

This comment has been minimized.

Copy link
Author

commented Jan 24, 2018

@doronsever
In case this is critical for you, I've just tested with editing the resulted ngsw-worker.js file and it works:
I've installed gulp and gulp-replace 😬 and created a gulp task that replaces onFetch(event) { with:

onFetch(event) {
    if (event.request.url.indexOf('/upload') !== -1) { return; }

in the dist/ngsw-worker.js file and then my build script became ng build [whatever options I have] && gulp swAjaxUploadFix (or I could have used the default task and simply run gulp, but I like to name my tasks).
Of course, in my case we have the convention that all upload urls contain /upload.
And also I log a big, multiple line message in the console on build so I don't forget about removing this workaround as soon as this feature gets implemented by the Angular team.
But, meanwhile, reporting the progress on uploads works! 😁
So, I just wanted to let you know it works. Because when I posted the issue, I had only read that this would fix progress reporting, and took what I read for granted, haven't tested myself. Now I have!

@doronsever

This comment has been minimized.

Copy link

commented Jan 24, 2018

@MrCroft Thank you very much!
At least it's working now :)

@ganeshkbhat

This comment has been minimized.

Copy link

commented May 5, 2018

Can this actually be put as a feature request from my end also? I think something having a key like "exclude" with interface like interface Exclude { name: string; urls: string[]; version?: number; }. There is one more open question like this in 'so. https://stackoverflow.com/questions/47693093/angular-5-and-service-worker-how-to-exclude-a-particular-path-from-ngsw-config

@gkalpak

This comment has been minimized.

Copy link
Member

commented May 5, 2018

The only problem I see is that the check should happen asynchronously. Based on #23025 (comment) it shouldn't be possible to decide whether to handle the request or not asycnhronously. But I am wondering if calling event.waitUntil() allows us to defer the decision of whether to call event.respondWith() or not and still be able to choose not to handle the request.

I.e. something like:

onFetch(evt) {
  evt.waitUntil(asynchronouslyDecideWhetherToHandle(evt).
    then(handleIt => handleIt && evt.respondWith(...));
}
@ganeshkbhat

This comment has been minimized.

Copy link

commented May 18, 2018

I see the point. If thats the case it definitely seems like its a tradeoff on performance and feature. May be someone is is more aware than me on the implementation can point the way forward.

@gkalpak gkalpak changed the title [ServiceWorker] Possibility to have urls that bypass the ServiceWorker (otherwise, Ajax progress listener doesn't work) feat(service-worker): Support ignoring specific URLs (e.g. for AJAX progress listener, video streaming, range requests) Sep 17, 2018

@gkalpak

This comment has been minimized.

Copy link
Member

commented Sep 17, 2018

This would also be useful as a workaround for Firebase uploads (#23244), video streaming (#25007) and range requests (e.g. seeking in videos) (#25865).
A possible solution could come through #20756 (comment).

@gkalpak gkalpak changed the title feat(service-worker): Support ignoring specific URLs (e.g. for AJAX progress listener, video streaming, range requests) feat(service-worker): Support ignoring specific URLs (e.g. for AJAX progress listener, video streaming, range requests, Firebase uploads) Sep 17, 2018

@GXGOW

This comment has been minimized.

Copy link

commented Dec 19, 2018

I'm having the same issue when watching server sent events through an EventStream. Bypassing the 'notifications' request with aforementioned workaround fixed it for now though.

@Delagen

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

This issue is blocking rendering PDF in Chrome from 71 version

@aSegatto

This comment has been minimized.

Copy link

commented Dec 21, 2018

We are also having problems with PDF documents, they are correctly served by the Service worker, but they don't get rendered. Bypassing the sw with the console allows to see the PDF document.

@webmaxru

This comment has been minimized.

Copy link
Member

commented Jan 1, 2019

Did you try specifying navigationUrlsexplicitly?
https://angular.io/guide/service-worker-config#navigationurls

@aSegatto

This comment has been minimized.

Copy link

commented Jan 6, 2019

Did you try specifying navigationUrlsexplicitly?
https://angular.io/guide/service-worker-config#navigationurls

what do you mean ? all files with extension are excluded with a glob; or am I missing something ?

@joaqcid

This comment has been minimized.

Copy link

commented Jan 18, 2019

as @gkalpak says, the service worker doesnt work on safari when uploading files with @angular/fire to firebase storage

@avilao

This comment has been minimized.

Copy link

commented Feb 12, 2019

Any roadmap on this issue?

@kamilchlebek kamilchlebek referenced this issue Mar 1, 2019

Closed

Fetch API cannot load: report-uri #1936

4 of 8 tasks complete
@Iyashu5040

This comment has been minimized.

Copy link

commented Mar 21, 2019

Plus one for this. We're hitting issues with the service worker adding "Cache-Control" headers to external API calls which don't allow that header. As we're not in control of the API we can't modify the accept headers on it. Therefore we need some way to have the browser do these API calls normally.

@petersalomonsen

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Very good point @jraadt

Added checking the url for the presence of ngsw-bypass=true:

private onFetch(event: FetchEvent): void {
    const req = event.request;
    if(req.headers.has('ngsw-bypass') || req.url.indexOf('ngsw-bypass=true') > -1) {
          return;
    }

Tested locally and seems to work. Looks ok?

@jraadt

This comment has been minimized.

Copy link

commented Apr 17, 2019

@petersalomonsen - That looks really good and it works in my tests.

I don't know if this matters, but I had an event.waitUntil() in my if statement. I just copied it from another section in the onFetch() that seemed to be bypassing the SW. Someone who knows more about how the SW works should weigh in, but this is what I have:

private onFetch(event: FetchEvent): void {
    const req = event.request;
    if(req.headers.has('ngsw-bypass') || req.url.indexOf('ngsw-bypass=true') > -1) {
          event.waitUntil(this.idle.trigger());
          return;
    }

@petersalomonsen

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Yes @jraadt - that seems to be needed so that update checks can occur. @gkalpak should probably confirm.

@gkalpak

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Great discussion. I think we are definitely on to something 😉
Supporting both a header and a query param SGTM (after having thought about it for 10 secs 😁)

Calling idle.trigger() shouldn't be necessary in this case (similar to what we do in other cases where we ignore specific requests).
(Not that it would hurt, but I'd leave it out for consistentcy - and because it is unnecessary 😁)

@Iyashu5040

This comment has been minimized.

Copy link

commented Apr 18, 2019

How would the header and query param solutions that have been proposed fix issues related to API calls made through SDKs where the developer is neither in control of the API or the SDK?

Case in point: The OneSignal Web Push SDK makes calls to the OneSignal API. The ngsw adds a Cache-Control header to the request, which the OneSignal API explicitly disallows. The developer cannot add special headers or query params to the request since it's being done through an SDK.

@gkalpak

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@lyashu5040, it won't be able to address those cases (but it will address cases all other cases).
It is not an ideal solution, but it is the best we've come up with so far 😁

@gkalpak

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

BTW, for anyone desperate for a work-around, I've seen people successfully wrapping ngsw-worker.js in their own script that catches fetch events early and stops their propagation to ngsw-worker.js. Off the top of my head, something like:

// Catch `fetch` events and prevent specific ones from propagating to the "real" SW".
self.addEventListener('fetch', evt => {
  const req = evt.request;

  if (/* `req` matches some criteria */) {
    evt.stopImmediatePropagation();
  }
});

// Import the "real" SW to do the heavy-lifting.
self.importScripts('./ngsw-worker.js');

If you do that, you might want to set updateViaCache: 'all', when registering the SW.

I am not sure this will work (and it is definitely not an officially recommended approach 😇), but might be good enough as a work-around.

petersalomonsen added a commit to petersalomonsen/angular that referenced this issue Apr 21, 2019

feat(service-worker): bypass on queryparam/header
issue angular#21191

added ngsw-bypass header and queryparameter
for bypassing serviceworker on specific requests
@petersalomonsen

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2019

Started work on a pull request: #30010 @gkalpak

petersalomonsen added a commit to petersalomonsen/angular that referenced this issue Apr 21, 2019

feat(service-worker): bypass on queryparam/header
issue angular#21191

added ngsw-bypass header and queryparameter
for bypassing serviceworker on specific requests

petersalomonsen added a commit to petersalomonsen/angular that referenced this issue Apr 21, 2019

feat(service-worker): bypass on queryparam/header
issue angular#21191

added ngsw-bypass header and queryparameter
for bypassing serviceworker on specific requests

petersalomonsen added a commit to petersalomonsen/angular that referenced this issue Apr 23, 2019

feat(service-worker): support bypassing SW with specific header/query…
… param

Add support for bypassing the ServiceWorker for a request by using the ngsw-bypass header or query
parameter.

Fixes angular#21191

petersalomonsen added a commit to petersalomonsen/angular that referenced this issue Apr 24, 2019

feat(service-worker): support bypassing SW with specific header/query…
… param

Add support for bypassing the ServiceWorker for a request by using the
ngsw-bypass header or query parameter.

Fixes angular#21191
@frankadrian

This comment has been minimized.

Copy link

commented Apr 24, 2019

I found a fairly simple workaround if you want your SW to completely "ignore" a specific path (for example we had a few paths that get redirected to another domain, which we did not want to be handled by our SW) :
By adding paths, you want to ignore, with a cacheConfig.strategy of "freshness" as a dataGroup the service worker will automatically load the content from the server (given that the timeout is not reached.)

eg ngsw-config.json:

...
"dataGroups": [
{
  "name": "cloudfront behaviours",
  "urls": [
    // for these paths the Service Worker is ignored
    "/terms",
    "/blog-posts"
  ],
  "cacheConfig": {
    "maxSize": 0,
    "maxAge": "0u",
    "strategy": "freshness"
  }
},
... 
@gkalpak

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

@frankadrian, this is something different. In this case, the SW will still handle the request, but it will just choose to forward it to the server and forward the response back to the app. This is already possible without specifying a data-group (i.e. is the difault behavior).

This thread is about having the SW not handle the request (and let the browser handle it instead). There is a subtle difference, but it can be important in certain cases (for reasons explained above).

petersalomonsen added a commit to petersalomonsen/angular that referenced this issue Apr 24, 2019

feat(service-worker): support bypassing SW with specific header/query…
… param

Add support for bypassing the ServiceWorker for a request by using the
ngsw-bypass header or query parameter.

Fixes angular#21191

petersalomonsen added a commit to petersalomonsen/angular that referenced this issue Apr 24, 2019

feat(service-worker): support bypassing SW with specific header/query…
… param

Add support for bypassing the ServiceWorker for a request by using the
ngsw-bypass header or query parameter.

Fixes angular#21191

petersalomonsen added a commit to petersalomonsen/angular that referenced this issue Apr 24, 2019

feat(service-worker): support bypassing SW with specific header/query…
… param

Add support for bypassing the ServiceWorker for a request by using the
ngsw-bypass header or query parameter.

Fixes angular#21191

gkalpak added a commit to petersalomonsen/angular that referenced this issue Apr 25, 2019

feat(service-worker): support bypassing SW with specific header/query…
… param

Add support for bypassing the ServiceWorker for a request by using the
ngsw-bypass header or query parameter.

Fixes angular#21191
@aSegatto

This comment has been minimized.

Copy link

commented Apr 26, 2019

Please don't close this. As was stressed before the current implementation doesn't fix all usecases, eg. when using firebase SDK.

@petersalomonsen

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

@aSegatto Have you tried using an HttpInterceptor: https://angular.io/api/common/http/HttpInterceptor

It could maybe capture the requests made by the firebase SDK? And in that case add the ngsw-bypass header.

Guide for using HttpInterceptor can be found here: https://angular.io/guide/http#intercepting-requests-and-responses

@gkalpak

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

@petersalomonsen, I don't think the Firebase SDK goes through the HttpClient (for all requests), but I might be wrong.

@aSegatto, fair enough. I suggest we open a new issue specifically for situations where using an HTTP header or query param is not possible (since that is slightly different than the original issue). Can you do that (and reference this issue for context)? (Also, I think it is worth mentioning this work-around for visibility: #21191 (comment))

@petersalomonsen

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2019

@aSegatto @gkalpak I've had a quick look at AngularFire and seems like it's not using HttpClient. But even when this is the case we can still intercept XMLHttpRequest for example like this:

const originalOpen = XMLHttpRequest.prototype.open;
XMLHttpRequest.prototype.open = function( method, url) { 
	originalOpen.apply(this,[method, url]);
	this.setRequestHeader('ngsw-bybass','true');
};

Then any library using XMLHttpRequest in your client will set the ngsw-bypass header. You can extend this to check for specific URLs / methods.

We could of course add another feature to the serviceworker by using the message port to send lists of URLs to be bypassed (and the serviceworker could store this in a map), but I think I would try out the client side interception strategy first.

@aSegatto

This comment has been minimized.

Copy link

commented Apr 29, 2019

@petersalomonsen Thanks for your suggestions. I don't know if modifying XMLHttpRequest would work , because i don't know how the requests are made from the firebase sdk (maybe they also use fetch API ? ), but this could be an approach...
Message port IMHO is more clean as it doesn't couple to the sdk implementation.

@gkalpak Thanks, I open another issue

bibyzan pushed a commit to bibyzan/stitch-js-sdk that referenced this issue May 2, 2019

Bennett Rasmussen
Added http header to BrowserFetchStreamTransport that bypasses servic…
…e worker onFetch implementation and lets the browser handle the request, specific to angular.

Background on root issue w3c/ServiceWorker#885
Issue leading to work around angular/angular#21191
Workaround that checks for the header petersalomonsen/angular@c7b357a

BioPhoton added a commit to BioPhoton/angular that referenced this issue May 21, 2019

feat(service-worker): support bypassing SW with specific header/query…
… param (angular#30010)

Add support for bypassing the ServiceWorker for a request by using the
ngsw-bypass header or query parameter.

Fixes angular#21191

PR Close angular#30010
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.