Inconsistent return value type from FileLoader.load()

[takahirox]

Description of the problem

FileLoader.load() returns three types of return value.

1. XMLHttpRequest instance when cache doesn't hit and url is not data: URI
2. cached data when cache hits
3. undefined when url is data: URI

As for me it'd be kinda confusing to users because the return value type
depends on the cache state and url type.
Another confusing thing is it returns cached(loaded) data synchronously
before it calls manager.itemEnd() when cache hits.

I like consistent design,
I want loaded/cached data only to be asynchronously passed to user via onLoad callback.

So how about returning null when 2. and 3.?
Or any other ideas?

Three.js version

• Dev

Browser

• All of them
• Chrome
• Firefox
• Internet Explorer

OS

• All of them
• Windows
• Linux
• Android
• IOS

Hardware Requirements (graphics card, VR Device, ...)

[mrdoob]

I agree. Maybe we can always return null?

[takahirox]

Yep. Or returning nothing is another option.

The reason why I didn't write we should return null/nothing
also for 1. in the previous post is just I'm worried about compatibility a bit.
There'd be users who locally make use of XMLHttpRequest
returned from FileLoader.load in their user code.

[satori99]

Is anyone considering Promises for this type of thing?

If loader.load() always returns a Promise that resolves the content, then it doesn't matter if the content comes from the cache object or an xml req. etc.

http://caniuse.com/#feat=promises

Support for Promises is very good already.

[takahirox]

Related filed issue #10644

Yeah, Promises is a good option but
I'm feeling like IE11 hasn't supported yet is kinda big concern...

[mrdoob]

I think @makc was the one that suggested returning XMLHttpRequest. I wonder what he has to say...

[makc]

I did suggest that to have a way to abort downloads. If you had an abort() method somewhere, returned value would not matter

[takahirox]

Hm, so will we return XMLHttpRequest instance for 1. and null for 2., 3. so far?
And probably we need request.addEventListener( 'abort', ... ) in FileLoader.load and
itemAbort in LoadingManager.

[makc]

that's only a part of the problem, since the reference is lost in many model loaders, iirc. I did #8099 for this, but it was sitting idle for too long, and I closed it because of merge conflicts.

[takahirox]

Hm, so small change first then we go further with two or three steps?

1. return null or XmlHttpRequest instance from FileLoader
2. return null or XmlHttpRequest instance from all model loaders
3. add abort or abortAll active requests function (in LoadingManager?) if necessary

Related PRs
#8099
#9600

[mrdoob]

I think I would vote for returning null and adding .abort() to FileLoader.

[takahirox]

Does .abort() abort all active requests of a FileLoader instance?
Or a certain one?

[mrdoob]

Good point...

[takahirox]

And probably FileLoader needs to manage active requests.
Or let LoadingManager do that.

[makc]

I think "abort all" in file loader could work if there was a way to make multiple end loaders use single file loader instance, but iirc there is no way. So it has to be in the manager