GLTFLoader: Set request header on FileLoader

[Zielon]

GLTFLoader class uses internally FileLoader however does not expose the method to set request headers, for instance the authorization header cannot be set. This PR fixes this problem by wrapping the method setRequestHeader().

Edit:
Additionally, GLTFParser uses a new instance of FileLoader internally. This prevents to download any assets using request headers.

[Mugen87]

I'm not sure we should start to expose this method, see #10609 (comment).

[Zielon]

If the method is not exposed there is not possibility to set authorization headers. Moreover, GLTFParser uses a different FileLoader internally. Thus, all protected assets are not available and cannot be downloaded.

[Mugen87]

Instead of changing GLTFLoader, it's also possible to enhance the base class Loader with requestHeader and setRequestHeader(). In GLTFLoader, you can then always set loader.setRequestHeader( this.requestHeader ); (without an if statement).

I think this is the more generic solution since this approach could be used in other loaders, too.

[Mugen87]

That looks nice now 😊. Let's see how @mrdoob thinks about this solution.

Reminder: It's also necessary to update the documentation (shifting the existing docs from FileLoader to Loader).

[Zielon]

After this PR is merged, I will have to adjust the rest of loaders to support a request header as well.

[mrdoob]

Looks good to me. I'll merge it after releasing r114.

[gkjohnson]

Any thoughts on putting this on the manager object? It seems in family with the url manipulation the manager handles and that way it can applied to all loaders that are using it:

const manager = new LoadingManager();
manager.setRequestHeaders( { ... } );

new GLTFLoader( manager ).load( ... );
new PLYLoader( manager ).load( ... );
new VTKLoader( manager ).load( ... );

[Mugen87]

One could do this but there is also Loader.setCrossOrigin(). So I think this solution here is fine.

[gkjohnson]

Right -- I actually feel like setCrossOrigin might be better suited on the manager, as well. It seems like it would be nice if the request management were consolidated in one spot and made easier to reuse across requests. Just a suggestion!

[mrdoob]

@gkjohnson how would the user code look like?

[gkjohnson]

@gkjohnson how would the user code look like?

Something like so:

const manager = new LoadingManager();
manager.setCrossOrigin( true );
manager.setRequestHeader( {

    'X-Hello': 'World'

} );

const loader = new GLTFLoader( manager );
loader.load( '../path/to/file.gtlf', model => {

} );

It just seems like if you need to add headers in order to load your models (credentials, etc) you're going to want it on every request and it would convenient to reuse the settings on a manager that can be shared.

[mrdoob]

@donmccurdy Do you have any thought on this?

[donmccurdy]

I can't think of a preference between putting the method on Loader vs. LoadingManager. However, I would suggest pluralizing the method name: setRequestHeaders. That phrasing is standard e.g. in Fetch API.

[Zielon]

I agree, however, since the name (setRequestHeader) is already used in three.js changing it now is not backward compatible.

[gkjohnson]

In my opinion it should go on LoadingManager because it's possible that one loader will kick off another loader that will need the same headers. Consider the case where you need to include an auth token in a request header to load a GLTF file and that GLTF file in turn loads an external texture file (which would need the same auth token). In this way it's similar to the resolveURL function which also gives you the opportunity to manipulate the request even for nested loader requests.

How would this work otherwise?

[Mugen87]

How would this work otherwise?

In the same way crossOrigin is passed to other loaders:

three.js/examples/js/loaders/GLTFLoader.js

Line 1411 in ed29076

this.textureLoader.setCrossOrigin( this.options.crossOrigin );

[Mugen87]

This PR is a simple and backwards compatible change. I don't think it should be blocked by the question if LoadingManager should perform tasks that are currently done by Loader. This can be changed later, too (if at all).

[gkjohnson]

This PR is a simple and backwards compatible change. I don't think it should be blocked by the question if LoadingManager should perform tasks that are currently done by Loader.

I didn't mean to block it just make a suggestion for a use case that didn't seem considered. I forgot that the setCrossOrigin pattern already existed. If that's how request headers should be handled then it should be included in the PR, no? Otherwise this won't work with models that load external files (the TextureLoader specifically).

[Mugen87]

If that's how request headers should be handled then it should be included in the PR, no?

Yes, that make sense.

Edit: I'ver realized right now that TextureLoader does not set request headers at all since it is derived from ImageLoader. Request headers are a FileLoader thing though.

Since the PR calls setRequestHeader() on all internal file loaders, no further changes are necessary.

[mrdoob]

Thanks!