R128 - Server Side Rendering issues

[RenaudRohlinger]

Hello!

Description
With the latest version (r128) I now get this error on an SSR architecture:

This PR fixes the issue by checking if the "document" exists.

[mrdoob]

Hmm, it'll be good to figure out what caused this 🤔

[RenaudRohlinger]

@mrdoob Well, it seems that rm -rf node_modules/ && yarn install fixed it. I suppose that was just a change between 127 and 128 build that went strange during the update. Sorry for bothering ^^'

[RenaudRohlinger]

@mrdoob Well, I did talk too fast, after further investigation differents errors with the same problem have appeared. It seems that's it might be related to examples now being maybe embedded with three.js?

So for example now this line:
https://github.com/mrdoob/three.js/blob/dev/examples/jsm/loaders/TiltLoader.js#L407
will call document in the image loader as soon as we import three.js.
--> new TextureLoader().setPath( './textures/tiltbrush/' ).load( 'Light.webp' )

This PR should fix that issue.

I'm trying to automate some SSR tests to prevent this type of issue:
pmndrs/drei#375

I'd be happy to implement it to three repo if we succeed to make a clean test process.

Related issue:
pmndrs/drei#376

[Mugen87]

I don't understand how this issue was introduced with r128. document is used in ImageLoader since the beginning.

It was investigated once to make the core node compatible here #20824 but the suggestion were not implemented due to the related complexity.

[drcmda]

this imo should be avoided, it's a side-effect:

const loader = new TextureLoader().setPath( './textures/tiltbrush/' );

const shaders = {
	'Light': {
		uniforms: {
			mainTex: { value: loader.load( 'Light.webp' ) },

there is no benefit here, it just prevents tree shaking and node. shaders is only used in getMaterial in the instance of TiltLoader, it probably would be for the best to do:

let shaders = null

function getMaterial( GUID ) {
  if (!shaders) {
    const loader = new TextureLoader().setPath( './textures/tiltbrush/' )
    shaders = { ... }
  }
  const name = BRUSH_LIST_ARRAY[ GUID ];

there's a very useful article about tree shaking here: https://twitter.com/AndaristRake/status/1384496467994456067 side-effects is the # 1 in that list, and this one is easy to avoid.

[joshuaellis]

@Mugen87, while what @drcmda is saying is correct, the use of ImageLoader in a the global scope will cause the error, it's not the problem of ImageLoader. So I don't think this PR is the correct approach. Fixing in TiltLoader is probably the better solution imo as it's a very isolated issue.

Here is a fix that i've applied to three-stdlib that resolves the issue.

This issue has probably been around since the loader was added, but because all our (pmndrs) libraries use three-stdlib and I had fixed the issue in March. Since r128 changed so many things to be ES Classes, drcmda did a job on moving files over to make three-stdlib compatible with three@128 undoing the work I did, and now we're here.

[Mugen87]

the use of ImageLoader in a the global scope will cause the error,

I do not yet understand this, sorry. The texture loading in TiltLoader happens in module scope. As long as the loader is not imported, I would assume this line is never executed (and hence tree-shaking should not be affected). Do you mind explaining this bit in more detail?

[joshuaellis]

So the file TiltLoader.js exports the class TiltLoader but there are variables in the file that are not encapsulated by a function or anything so these variables:

const loader = new TextureLoader().setPath( './textures/tiltbrush/' );
const shaders = {
	'Light': {
		uniforms: {
			mainTex: { value: loader.load( 'Light.webp' ) },
			alphaTest: { value: 0.067 },
			emission_gain: { value: 0.45 },
			alpha: { value: 1 },
		},
		vertexShader: `
			precision highp float;
			precision highp int;
			attribute vec2 uv;
			attribute vec4 color;
			attribute vec3 position;
			uniform mat4 modelMatrix;
			uniform mat4 modelViewMatrix;
			uniform mat4 projectionMatrix;
			uniform mat4 viewMatrix;
			uniform mat3 normalMatrix;
			uniform vec3 cameraPosition;
			varying vec2 vUv;
			varying vec3 vColor;
			${ common.colors.LinearToSrgb }
			${ common.colors.hsv }
			void main() {
				vUv = uv;
				vColor = lookup(color.rgb);
				vec4 mvPosition = modelViewMatrix * vec4( position, 1.0 );
				gl_Position = projectionMatrix * mvPosition;
			}
		`,
		fragmentShader: `
			precision highp float;
			precision highp int;
			uniform float emission_gain;
			uniform sampler2D mainTex;
			uniform float alphaTest;
			varying vec2 vUv;
			varying vec3 vColor;
			${ common.colors.BloomColor }
			${ common.colors.SrgbToLinear }
			void main(){
				vec4 col = texture2D(mainTex, vUv);
				vec3 color = vColor;
				color = BloomColor(color, emission_gain);
				color = color * col.rgb;
				color = color * col.a;
				color = SrgbToLinear(color);
				gl_FragColor = vec4(color, 1.0);
			}
		`,
		side: 2,
		transparent: true,
		depthFunc: 2,
		depthWrite: true,
		depthTest: false,
		blending: 5,
		blendDst: 201,
		blendDstAlpha: 201,
		blendEquation: 100,
		blendEquationAlpha: 100,
		blendSrc: 201,
		blendSrcAlpha: 201,
	}

};

are evaluated before the function getMaterial is even called. So, the Loader is initialised to create the variable loader and then loader.load is called when evaluating the variable shaders so the reference can be read by getMaterial, variables aren't created when they're referenced.

Edit: link to code in three here

[drcmda]

I do not yet understand this, sorry. The texture loading in TiltLoader happens in module scope. As long as the loader is not imported, I would assume this line is never executed

additionally to what josh said, the line is executed on the server, and there it fails. this currently affects all SSR solutions: next, gatsby, etc. also testing of course.

as for tree shaking, that will also fail as a separate issue, once you start to flat bundle jsm. i think generally it's good practice to keep an eye on side-effects in global or module space - there are only downsides.

[Mugen87]

Thanks for the additional details! I've filed a PR based on the suggestions of this thread.

[Mugen87]

Closing in favor of #21721.

[joshuaellis]

No worries, glad we could all help!