w3c / ServiceWorker

Service Workers
https://w3c.github.io/ServiceWorker/
Other
3.63k stars 314 forks source link

Service Worker makes AJAX Progress Listener not Working #1141

Open albertsylvester opened 7 years ago

albertsylvester commented 7 years ago

Hi everyone,

With Service Worker (SW) installed on a web apps, it caused "AJAX Progress Listener" not working. Any idea?

Tested in Chrome Desktop, Chrome Mobile v 58.0, Firefox Desktop v 52.0.2, Firefox Android v 53.0.2 It's working Thanks in advance

The Service Worker

if ('serviceWorker' in navigator && (window.location.protocol === 'https:')) {
        navigator.serviceWorker.register('/service-worker.js')
        .then(function(registration) {
          //Checks the server for an updated version of the service worker without consulting caches.
          //registration.update();

          // updatefound is fired if service-worker.js changes.
          registration.onupdatefound = function() {
            // updatefound is also fired the very first time the SW is installed,
            // and there's no need to prompt for a reload at that point.
            // So check here to see if the page is already controlled,
            // i.e. whether there's an existing service worker.
            if (navigator.serviceWorker.controller) {
              // The updatefound event implies that registration.installing is set:
              // https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#service-worker-container-updatefound-event
              var installingWorker = registration.installing;

              installingWorker.onstatechange = function() {
                switch (installingWorker.state) {
                  case 'installed':
                    // At this point, the old content will have been purged and the
                    // fresh content will have been added to the cache.
                    // It's the perfect time to display a "New content is
                    // available; please refresh." message in the page's interface.
                    console.warn('New content is available, please refresh the page');
                    break;

                  case 'redundant':
                    throw new Error('The installing ' +
                                    'service worker became redundant.');

                  default:
                    // Ignore
                }
              };
            }
          };
        }).catch(function(e) {
          console.error('Error during service worker registration:', e);
        });
      }

And, the Ajax Uploader

function uploadPic(){
        var file = document.getElementById('fileInput').files[0];
        var fd = new FormData();
        fd.append("file", file);

        var xhr = new XMLHttpRequest();

        //attach listener before posting
        xhr.upload.onprogress = updateProgressBar;

        xhr.onreadystatechange = function() {//Call a function when the state changes.
            if(xhr.readyState == 4 && xhr.status == 200) {
                console.log(xhr.responseText);
            }
        }

        xhr.open("POST", "https://www.mysite.com/upload.php", true);
        xhr.send(fd);
      }

      function updateProgressBar(e){
        if (e.lengthComputable) {
          var percentComplete = (e.loaded / e.total) * 100;
          console.log(percentComplete + '% uploaded', e);
        }
      }
wanderview commented 7 years ago

This is about progress on an upload body, right?

The progress may not be right if the service worker script clones or reads the FetchEvent.request's body. Are you doing that? Can you post your fetch event handler?

Besides that, I am willing to bet service worker implementations have bugs related to this. In gecko I am fairly sure we don't pipe back progress from the network socket, through the SW, back to the original XHR. In theory this could be possible if you do a pass-through like fetch(event.request).

wanderview commented 7 years ago

In gecko I think progress should work if you avoid calling respondWith() for these requests in your service worker.

albertsylvester commented 7 years ago

Hi wanderview,

thanks for the answer. Yeah, I think I know the answer. It is because of the fetch API in my SW.

service-worker.js (Fetch Section)

self.addEventListener('fetch', function(event) {
  console.log('Handling fetch event for', event.request.url, version);

  event.respondWith(
    caches.match(event.request).then(function(response) {
      if (response) {
        //console.log('Found response in cache:', response);
        return response;
      }
      //console.log('No response found in cache. About to fetch from network...', event.request.url);

      return fetch(event.request).then(function(response) {
        //console.log('Response from network is:', response);
        return response;
      }).catch(function(error) {
        console.error('Fetching failed:', error);
        throw error;
      });
    })
  );
});

Here's a reference: https://github.com/github/fetch/issues/89

wanderview commented 7 years ago

@albertsylvester Since Cache API never stores "POST" requests, you could put this at the top of your fetch event handler to avoid the respondWith():

if (event.request.method === 'POST') {
  return;
}

I'm not sure we can make the pass-through fetch(event.request) work for progress until we fix progress in the Fetch API spec.

albertsylvester commented 7 years ago

Thanks @wanderview. You are awesome. Why do you want to avoid respondWith()? Thank you

wanderview commented 7 years ago

Why do you want to avoid respondWith()?

It allows the network request to complete just like it would without the service worker (in theory).

wanderview commented 7 years ago

Sorry, I think maybe we should keep this open to track the problem with progress when using a pass-through service worker.

albertsylvester commented 7 years ago

Ok @wanderview . Thanks

jakearchibald commented 6 years ago

@wanderview which part of the spec is causing this to break currently?

wanderview commented 6 years ago

Uh, fetch API doesn't have any progress API yet? Don't we need FetchObserver for that? Until we have that we can't really propagate progress through the respondWith() accurately. The best you can do is use a ReadableStream for the upload and assume when its read its been uploaded, but re-buffering can be taking place.

jakearchibald commented 6 years ago

I thought, because of the way the stream was adopted, https://fetch.spec.whatwg.org/#concept-body-transmitted would still update, but I need to take a closer look. Fetch observing would certainly make this clearer.

wanderview commented 6 years ago

I wasn't aware of the "transmitted bytes" language in fetch upload stream spec. I guess it might be adequate for the progress issue, but it also kind of seems like a de-opt for upload throughput to me. It doesn't really match actual browser implementation (at least ours) very well.

isocroft commented 6 years ago

@jakearchibald would it be possible to create a polyfill using the stream object for upload progress within/for the Fetch API ??

From this: https://github.com/yutakahirano/fetch-with-streams/

It seems a bit complicated