WebGPURenderer: Fix global references in Node.js #29919
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
sunag
changed the title
react
donmccurdy
changed the title
react
|
Note – I've renamed the issue to "fix global references in Node.js". I think that's the issue here, not React. Next.js is a React-based framework that happens to execute code in both a browser environment and server-side in Node.js. |
| @@ -64,7 +64,7 @@ class WebGPUBackend extends Backend { | |||
| powerPreference: parameters.powerPreference | |||
| }; | |||
|
|
|||
| const adapter = await navigator.gpu.requestAdapter( adapterOptions ); | |||
| const adapter = ( typeof navigator !== 'undefined' ) ? await navigator.gpu.requestAdapter( adapterOptions ) : {}; | |||
There was a problem hiding this comment.
@donmccurdy{} is intended as a workaround for node but I'm not sure {} is ideal here. Do you think this use case is handled appropriately or would you implement it in a different way?
There was a problem hiding this comment.
Base on that function code, it should be null, otherwise, next step is exceptional?
"adapter.features"
There was a problem hiding this comment.
@whatisor are you executing this code in the Node.js environment and seeing runtime errors, or is this a compile-time error (like from TypeScript or Webpack)? Not clear to me from #29916.
I'm not sure that backend.init( renderer ) should be making efforts to avoid runtime exceptions, if running in an environment where neither WebGL nor WebGPU exists. I would support fixes that avoid build-time errors, and run-time errors in module scope. But if you are initializing and using a renderer in Node.js, a runtime exception is to be expected.
There was a problem hiding this comment.
I'm not sure that backend.init( renderer ) should be making efforts to avoid runtime exceptions, if running in an environment where neither WebGL nor WebGPU exists
I agree, I think we should be throwing an error here, it doesn't make much sense to me to proceed with an empty object as an adapter.
There was a problem hiding this comment.
@donmccurdy it is compile error.
We also need to fix three\examples\jsm\capabilities\WebGPU.js with global and await.
So, we should return null and (exception is next step as current)?
There was a problem hiding this comment.
three\examples\jsm\capabilities\WebGPU.js
Capabilities should be optional, it should be possible to use WebGPURenderer without global await. #29218
There was a problem hiding this comment.
can we do same thing with webgl to avoid await?
https://developer.mozilla.org/en-US/docs/Web/API/GPUCanvasContext
=> No, we still need to await for real adapter. I guess browser need to improve this part.
Use `null` as fallback when navigator isn't available.
| @@ -113,13 +113,21 @@ class WebGPUUtils { | |||
| // TODO: Remove this check when Quest 34.5 is out | |||
| // https://github.com/mrdoob/three.js/pull/29221/files#r1731833949 | |||
|
|
|||
| if ( navigator.userAgent.includes( 'Quest' ) ) { | |||
| if ( typeof navigator !== 'undefined' ) { | |||
There was a problem hiding this comment.
Could we remove this? It is inside a function that will never be executed by the adapter return null in this case.
There was a problem hiding this comment.
I guess the issue is that it's a compiler error, not a runtime error? I wish we knew what in the Next.js stack is doing compile-time checks on dependencies, and what its requirements are. Avoiding all references to browser globals is a pretty tall order for a large library relying heavily on Web APIs. :/
There was a problem hiding this comment.
The compiler throwing an error for a global variable inside a function seems strange to say the least, since a variable can be declared at the global level after the function declaration.
There was a problem hiding this comment.
Yeah, I'm not sure why... trying to reproduce the issue:
There was a problem hiding this comment.
I think it is this one:
https://react.dev/reference/react/StrictMode
|
I have set up a test suite of various build environments, like Vite, with and without React, Next.js, R3F, and a specific one to check this PR: https://github.com/verekia/three-gpu-ecosystem-tests The PR does indeed fix Next.js crashing on reference errors 🙌 That being said, I am getting a Regular import (fixed by this PR): Dynamic import workaround: |
Fix #29916.
Description
Fix compilation issue on react.