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

Use Fetch in FileLoader #22510

Merged
merged 9 commits into from
Oct 6, 2021
Merged

Conversation

DefinitelyMaybe
Copy link
Contributor

@DefinitelyMaybe DefinitelyMaybe commented Sep 8, 2021

Related issues: #22468, #21286

Use fetch in FileLoader

Related comment

@mrdoob mrdoob added this to the r??? milestone Sep 10, 2021
@gkjohnson
Copy link
Collaborator

Uncaught TypeError: Cannot read properties of undefined (reading 'lengthComputable')
I haven't cover the [what should be passed to the callbacks for the progress event] yet. With XMLHttpRequest() it was an event. The funny side of this is that either that's the only or one-of-the-only loaders using the event tehe

I forgot about the onProgress callback 😁. In theory this callback is useful for applications displaying a more granular download progress bar (though admittedly I haven't seen it used in practice I don't believe). I think the onProgress event should be passed an object similar to the ProgressEvent object (lengthComputable, total, loaded members) for backward compatibility.

You should be able to use the "ReadableStreams" API to progressively read chunks of the response body to get load progress and look at the response headers to check the content-length header. Here's a writeup on using StreamReader from MDN and here's the caniuse for ReadableStream, which looks pretty good.

One last thing to note is that XMLHTTPRequest.responseType can be set to "document" to return a pre-parsed XML result (docs here. In terms of regressions setting the responseType to "document" is something that would have been supported previously thought I don't believe any loaders use it, it is documented as an option for FileLoader. I'm not sure if any users would be, though.

@DefinitelyMaybe
Copy link
Contributor Author

Could ReadableStreams be popped into:

switch ( this.responseType ) {
  case 'arraybuffer':
  // ...
  case 'stream':
    return res.body.getReader();

Backwards compatibility is a good idea and we should probably stick to that. What was the thinking behind the onProgress event and how granular are we talking?

In this PR the onProgress event is equivalent to saying "the fetch has started but you'll have to wait for the responseType you requested", it could've just as easily been "the fetch has succeeded but the response is so big that you should stream it". There's other ways of thinking what progress means too so... yeah. What would we like?

@gkjohnson
Copy link
Collaborator

What was the thinking behind the onProgress event and how granular are we talking?

The goal would be to provide progress callbacks consistent with XMLHTTPRequest so the granularity is up to the browser and how often the reader callback fires. It shouldn't be necessary to add new responseTypes to support it.

I think it's best to get familiar with the current behavior of the onProgress callback. You can check out the MDN docs for the XMLHTTPRequest "progress" callback here. The event can get fired multiple times over the course of a download with updates to download percent and gets fired regardless of the set responseType.

@DefinitelyMaybe
Copy link
Contributor Author

@gkjohnson Could you help me with sorting out the progress properly. I think this is approximately what we're looking for but I can't just throw it above the switch ( this.responseType ) { because otherwise the return response.arrayBuffer(); statements will error out as the body is already read.

const callbacks = loading[ url ];
const reader = response.body.getReader()
let loaded = 0
let total = parseInt(response.headers.get("Content-Length"))

const progress = reader => {
return reader.read()
	.then(({done, value}) => {
		if (done) return;
		loaded += value.length
		const event = new ProgressEvent('progress', {lengthComputable: true, loaded, total})
		for ( let i = 0, il = callbacks.length; i < il; i ++ ) {

			const callback = callbacks[ i ];
			if ( callback.onProgress ) callback.onProgress(event);

		}
		return progress(reader);
	});
}
progress(reader)

@gkjohnson
Copy link
Collaborator

gkjohnson commented Sep 13, 2021

From the MDN writeup I linked you can structure a fetch to create a new response object like so:

fetch( 'url.ext' )
    .then( response => {
        const reader = response.body.getReader();
        let loadedData = 0;

        // periodically read data into the new stream tracking while download progress
        return new ReadableStream( {
            start( controller ) {

                readData();

                function readData() {

                    reader.read().then( info => {
                        if ( info.done ) {

                            controller.close()

                        } else {

                            loadedData += info.value.byteLength;

                            /*
                            * fire on progress event here
                            */

                            controller.enqueue( info.value );
                            readData();
                        }
                    } );

                }
            }
        } );
    } )
    .then( stream => {

        // create a new response and read the data as the requested format
        const response = new Response( stream );
        switch ( responseType ) {
            case 'blob':
                return response.blob();
            /* ... */
        }

    } );

I wonder if there are any performance implications to using this setup, though. @mrdoob are there any browser-knowledgeable people to get in touch with who would know if there are any pitfalls here compared to XMLHTTPRequest?

@DefinitelyMaybe
Copy link
Contributor Author

thank you @gkjohnson, I was looking into that article but my brain was struggling to wrap itself around the return new ReadableStream( { ... } ) part and how that worked. I haven't dived into using Streams before.

@DefinitelyMaybe DefinitelyMaybe marked this pull request as ready for review September 20, 2021 21:46
src/loaders/FileLoader.js Outdated Show resolved Hide resolved
src/loaders/FileLoader.js Outdated Show resolved Hide resolved
@mrdoob mrdoob modified the milestones: r???, r133 Sep 22, 2021
@mrdoob mrdoob added this to the r134 milestone Sep 30, 2021
@mrdoob
Copy link
Owner

mrdoob commented Oct 6, 2021

Alright, lets see how this goes! 🤞

@mrdoob mrdoob merged commit 8b80974 into mrdoob:dev Oct 6, 2021
@mrdoob
Copy link
Owner

mrdoob commented Oct 6, 2021

Thanks!

@DefinitelyMaybe DefinitelyMaybe deleted the Use-Fetch()-in-FileLoader branch October 6, 2021 22:06
@makc
Copy link
Contributor

makc commented Oct 29, 2021

@mrdoob

lets see how this goes!

not quite stellar, now my progress bars are broken. because, unlike with XHR, the server now has to explicitly give permission to read 'Content-Length' header - see https://stackoverflow.com/a/48266945

@DefinitelyMaybe
Copy link
Contributor Author

bummer. Not sure what we could use in the way of alternatives for getting the length. Any suggestions?

@donmccurdy
Copy link
Collaborator

Good to know about the HTTP header requirement.

One benefit of the change to fetch() that I wanted to mention, since it's otherwise buried in a resolved comment — Safari does not support Data URIs with XmlHttpRequest, but does support them in fetch(). This means we were able to remove the special handling of Data URIs and no longer need to block the main thread to decode them.

@makc
Copy link
Contributor

makc commented Oct 29, 2021

@DefinitelyMaybe

what we could use in the way of alternatives for getting the length. Any suggestions?

try xhr head request, maybe it will work (also the problem is limited to cross-origin requests, so you might want to match url to window.location)

@gkjohnson
Copy link
Collaborator

This doesn't seem to be how MDN describes the behavior of Access-Control-Expose-Headers, though. According to those docs it's only relevant for CORS requests:

The Access-Control-Expose-Headers response header allows a server to indicate which response headers should be made available to scripts running in the browser, in response to a cross-origin request.

And even then there is a safe list of headers that should be exposed by default even when Access-Control-Expose-Headers isn't specified which includes Content-Length. Are we sure there isn't something else going on here? Is this a browser-specific issue?

@makc can you put together a jsfiddle repro?

@makc
Copy link
Contributor

makc commented Oct 29, 2021

@makc
Copy link
Contributor

makc commented Oct 29, 2021

actually now when I test it again, I only get 0 in firefox - chrome does return the size

@makc
Copy link
Contributor

makc commented Oct 29, 2021

safari also reports 0

@gkjohnson
Copy link
Collaborator

gkjohnson commented Oct 29, 2021

actually now when I test it again, I only get 0 in firefox - chrome does return the size

Which version of Firefox are you using? I'm seeing the fiddle work in both Firefox (v94.0) and Chrome (94.0.4606.81) on Windows 10.

@makc
Copy link
Contributor

makc commented Oct 29, 2021

😔 good point, I need to re-test on more recent os

@CodyJasonBennett
Copy link
Contributor

CodyJasonBennett commented Nov 1, 2021

An issue I'm facing with this is that polyfills like GitHub's fetch don't have streaming support (related issue), and those that do implement it in a very limited capacity. That can pair with this PR to break in environments like react-native/node which depend on that polyfill or relied on XMLHttpRequest.

Is there a way to fake the old FileLoader or add backwards-compat here? Alternatively, and somewhat offtopic, is there a good polyfill for this that follows the spec?

@gkjohnson
Copy link
Collaborator

From that issue it looks like a fork has been produced to support the react-native use case with streams, though it looks like it may only be limited to supporting text:

https://www.npmjs.com/package/react-native-fetch-api

And if node is something we want to support we could only use a ReadableStream if an "onProgress" callback is provided and is supported by the browser so at least it doesn't fail in contexts without ReadableStream support.

@lafflan
Copy link
Contributor

lafflan commented Sep 16, 2022

Fetch API cannot load 'file' url scheme.
github/fetch#92

Such as use in webview

@towhare
Copy link
Contributor

towhare commented Sep 16, 2022

Fetch API cannot load 'file' url scheme. github/fetch#92

Such as use in webview

I use three.js in puppeteer to load file in local too. although I set '--disable-web-security' in browser. fetch API still can't fetch URL scheme
it will show error 'URL scheme "file" is not supported'.

@LeviPesin
Copy link
Contributor

Use --allow-file-access-from-files?

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

Successfully merging this pull request may close these issues.

10 participants