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

Loaders - ObjectLoaders and other example loaders giving TypeError in react-native #23015

Closed
AlanSmithGit opened this issue Dec 13, 2021 · 17 comments

Comments

@AlanSmithGit
Copy link

[TypeError: undefined is not an object (evaluating ‘response.body.getReader’)]

Above error is produced when load function of any loader ( like GLTFLoader or ObjectLoader ) is called.

Platforms tested on:

Mobile - iPhone
OS: iOS, MacOS
Three.js version 0.135.0
expo-three version 6.0.1
expo-gl version 11.0.3
expo version 43.0.2
react version 17.0.1
react-native version 0.64.3

Sample Code:

const asset = Asset.fromModule(
    require("./assets/v_knife_karam/v_knife_karam.gltf")
  );
  await asset.downloadAsync();

  const loader = new GLTFLoader();

  loader.load(
    asset.uri,
    (gltf) => {
      // model = gltf.scene;
      scene.add(gltf.scene);
    },
    (xhr) => {
      console.log(`${(xhr.loaded / xhr.total) * 100}% loaded`);
    },
    (error) => {
      console.error("An error happened", error);
    }
  );

Error : [TypeError: undefined is not an object (evaluating ‘response.body.getReader’)]

@CodyJasonBennett
Copy link
Contributor

See #23011 (comment). This is related to a regression made in r134.

@gkjohnson
Copy link
Collaborator

Like I mentioned in #22510 I'd think it should be possible to skip using the stream reader if it's not supported in the environment. Something like this at the beginning of the initial fetch callback:

if ( typeof ReadableStream === 'undefined' || response.body.getReader === undefined ) {

  // cannot support onProgress callbacks
  return response;

}

// otherwise the readable streams and onProgress callbacks are supported

It would be best if someone who's familiar with the environments where it's failing can make the PR and test to ensure the proposed solution works.

@CodyJasonBennett
Copy link
Contributor

Awesome. I'll dig around and see if I can get a PR up or at least a test case to work with.

@CodyJasonBennett
Copy link
Contributor

Seems this is a deeper issue -- response#arrayBuffer, etc. would be another breakpoint (snippet source):

}).then(stream => {
	const response = new Response(stream);

	switch (this.responseType) {
		case 'arraybuffer':
			return response.arrayBuffer();

		case 'blob':
			return response.blob();

Is there a way we could gracefully fallback to XMLHttpRequest? I don't want to introduce code paths but compatibility is a bit shakey with fetch.

Maybe we should continue in another issue if this needs more eyes.

@gkjohnson
Copy link
Collaborator

Seems this is a deeper issue -- response#arrayBuffer, etc. would be another breakpoint (snippet source):

Which fetch polyfill is being used? At this point I'd submit an issue / fix to the polyfill library. In the Github polyfill, for example, response.arrayBuffer is only available if Blob is supported by the environment which shouldn't be necessary. Or I'd think you could provide a global Blob polyfill like this one I found so the fetch polyfill can leverage that implemenation.

ReadableStreams are a lot newer but with the fetch API being stable for 6 years now I don't think I support addressing this in three.js. I feel if libraries are going to enable running browser code in non browser contexts then they need to support long stable and commonly used browser features.

@CodyJasonBennett
Copy link
Contributor

CodyJasonBennett commented Dec 13, 2021

Which fetch polyfill is being used? At this point I'd submit an issue / fix to the polyfill library. In the Github polyfill, for example, response.arrayBuffer is only available if Blob is supported by the environment which shouldn't be necessary. Or I'd think you could provide a global Blob polyfill like this one I found so the fetch polyfill can leverage that implementation.

I tested on react-native to see if it would resolve this issue, and that uses the Github polyfill. I think that's something for react-native to fix themselves if they're going to try to support fetch.

ReadableStreams are a lot newer but with the fetch API being stable for 6 years now I don't think I support addressing this in three.js. I feel if libraries are going to enable running browser code in non-browser contexts then they need to support long stable and commonly used browser features.

I don't understand. Wasn't XMLHttpRequest that or do we just tell people to use r133? I thought this was somewhat reversible.

@gkjohnson
Copy link
Collaborator

I don't understand. Wasn't XMLHttpRequest that or do we just tell people to use r133? I thought this was somewhat reversible.

I'm just suggesting that I don't think it's reasonable to expect three to hold on to such long outdated APIs because react native doesn't support them but that's just my opinion. If even basic fetch features such as res.arrayBuffer() aren't working I think the right place to fix this would be in react-native and/or the whatwg-fetch package. Ultimately the maintainers will have to weigh in, though.

I don't know exactly how react native works but as a workaround I'd expect you can override the global fetch polyfill with your own that implements whats needed in the mean time. Or you could override THREE.FileLoader.prototype.load with an implementation from before r133.

@CodyJasonBennett
Copy link
Contributor

Is XMLHttpRequest support really too much of an ask? I can put together a draft PR to look at, but I couldn't disagree more with the notion of alienating users who don't support an API if it's easily preventable.

react-native is just pertinent to this issue, but if there's no easy solution, then people can revert to r133.

@gkjohnson
Copy link
Collaborator

I feel what I've said is being twisted. I'm not advocating for alienating users... I'm advocating for this being fixed in the right location which I think is pretty clearly the polyfill or react-native. Can you help me understand why you're so resistent to getting it fixed there? The React Native docs even recommend referencing the MDN docs on fetch for how to use it so it seems like a clear bug if response.arrayBuffer can't be used. Is there an issue in either project already to enable proper behavior in the future?

If XMLHttpRequest support is going to be readded then I'd think we should just revert the FileLoader fetch change and not maintain two code paths.

@CodyJasonBennett
Copy link
Contributor

I feel what I've said is being twisted. I'm not advocating for alienating users... I'm advocating for this being fixed in the right location which I think is pretty clearly the polyfill or react-native. Can you help me understand why you're so resistent to getting it fixed there? The React Native docs even recommend referencing the MDN docs on fetch for how to use it so it seems like a clear bug if response.arrayBuffer can't be used. Is there an issue in either project already to enable proper behavior in the future?

I didn't mean to suggest that, but it doesn't feel right to send these people elsewhere without exhausting our options. I thought this was an unnecessary breaking change that was only causing problems for native and older platforms, so I wanted to inquire about the motivations for the change and if we could improve support.

As for issues, response.arrayBuffer would rely on JakeChampion/fetch#992, but it looks like native's networking stack would need some work for full compliance. There are polyfills available like react-native-polyfill-globals, but this is something for native to fix themselves. I suppose we can point react-native users here in the meantime, but it's unclear what options exist for other platforms.

If XMLHttpRequest support is going to be readded then I'd think we should just revert the FileLoader fetch change and not maintain two code paths.

I was hoping this wouldn't be a hassle, but if it is, then I think this is something that can live in three-stdlib. They already target node, so it should be fine to move FileLoader there.

@gkjohnson
Copy link
Collaborator

I thought this was an unnecessary breaking change that was only causing problems for native and older platforms

I think it's more accurate to say that three.js made a change that exposed a bug in react-native / the polyfill. In terms of the benefits of the change the fetch implementation simplified the handling of data URIs in the FileLoader, is easier to follow, and personally I'd like to see Loaders take a set of fetch options in the future considering how much more ergonomic they are.

As for issues, response.arrayBuffer would rely on JakeChampion/fetch#992, but it looks like native's networking stack would need some work for full compliance.

Have you created an issue for react-native, then? Again this is clearly a shortcoming of these other packages and limits the broader js ecosystems ability to leverage stable, modern APIs. Asking libraries to stall modernization to support workarounds for issues in other packages with no plan or effort to alleviate it is hard to support.

I was hoping this wouldn't be a hassle, but if it is, then I think this is something that can live in three-stdlib. They already target node, so it should be fine to move FileLoader there.

What I'm really asking for is for some effort to be made to fix the fetch polyfill and react native here instead of expecting three.js to revert because it's the easy way out. I think instead this effort is best spent making a PR for github fetch to fix the res.arrayBuffer issue and informing or making a PR for react-native to improve the fetch behavior if that's needed there too. Seeing what kind of progress can or can't be made on either project I'd think would better inform decision made by three.js.

And to be clear -- like I've said before -- this ultimately isn't up to me but these are my opinions.

@CodyJasonBennett
Copy link
Contributor

If XMLHttpRequest isn't helpful to platforms three cares about, then it shouldn't be considered. That's just me being ignorant. I thought that could be some sort of band-aid, but it will introduce a code path and is much slower than fetch.

I've already outlined WIP solutions and community implementations for react-native users to leverage fetch features if that's the only outstanding issue.

For Node users, I'm unsure whether more work is needed than #23015 (comment) and if similar problems will arise there, so I'll have to leave this to someone more knowledgeable. If Node isn't a platform that three supports, then this can move to three-stdlib.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 16, 2021

If Node isn't a platform that three supports

three.js does not officially support node environments. As mentioned in the documentation (section Node.js), the engine is designed for the web and relies on Web APIs which are not necessarily supported in node.js.

That said, we try make a node integration easier with PRs like #10732 or #14914 but only if the changes do not affect the code base too much.

In this particular case, the issue needs to be solved elsewhere like mentioned in #23015 (comment).

@neciszhang
Copy link
Contributor

I use it in web, i don't use it in react-native.I personally think that it should be fixed and it is unreasonable for users to go back to the 133 version.

@mrdoob
Copy link
Owner

mrdoob commented Dec 28, 2021

@neciszhang What exactly doesn't work when using it in web?

@neciszhang
Copy link
Contributor

neciszhang commented Dec 29, 2021

Hi Mr.Doob,

  1. 136version online demo: here. I open it in AliPay Browser,it's load .glb file failed.

image

  1. 133version online demo: here.I open it in Alipay Browser.It's load successfully.

image

@gkjohnson
Copy link
Collaborator

I'm unfamiliar with the Alipay web browser but is it possible that the response does not have a body field? Can you log the following on the FileLoader line in question:

console.log( typeof ReadableStream, response.body );

#23032 would have fixed this for the case where response.body.getReader is undefined but not if response.body is undefined.

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

No branches or pull requests

6 participants