Suggestion: Keep gl constants in build/three.js

[takahirox]

Description of the problem

We convert gl constants to their actual values like gl.TRIANGLES to 4 when we build build/three.js and build/three.min.js #15061.

But gl constants are more readable. So how about keeping gl constants in build/three.js and converting only in build/three.min.js?

I think build/three.js is often used for debug. If user face an error, one wants to jump to the line causing error in build/three.js by clicking three.js:linenumber from their console and/or wants to step over the code with debugger. Readable seems better for that case.

Three.js version

• Dev
• r102
• ...

Browser

• All of them
• Chrome
• Firefox
• Internet Explorer

OS

• All of them
• Windows
• macOS
• Linux
• Android
• iOS

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

[Mugen87]

I guess this would make the build configuration more complex since the closure compiler uses build/three.js as its input. npm run build-closure would need a temporary version of three.js so the compiler works with the correct version (without constants). Not sure it's worth doing this...

[Mugen87]

Maybe using a configuration parameter replaceGLConstants is easier to implement. It would be true by default and controls whether WebGL constants are replaced in build files or not.

[mrdoob]

If we switched to https://www.npmjs.com/package/google-closure-compiler-js, not only we could ditch Java, but we could also implement what @takahirox suggest right inside rollup.config.js?

[Mugen87]

but we could also implement what @takahirox suggest right inside rollup.config.js?

Interesting. But why is this possible with the JS-version of google-closure-compiler (and not with the Java version)? I thought using the JS version just means you have no dependency to Java anymore.

[mrdoob]

Hmm, because we can call google-closure-compiler-js from inside rollup.config.js?

[Mugen87]

Okay I see. I was not aware of such a usage:

https://www.npmjs.com/package/google-closure-compiler#javascript-version-1

[TimvanScherpenzeel]

I've been using a constant definition file like this in my engine based on the MDN definitions: https://gist.github.com/TimvanScherpenzeel/1f4218a563789db17955a1926c8a8ee0

On build the constants are inlined by the minifier (Terser).

[Mugen87]

I think build/three.js is often used for debug.

I'm not sure if this is still valid (at least in context of the repository) because of the module migration. All example use three.module.js now which is targeted by the constant replacement. Since this file is also used in production, I think I would not apply the suggested change to it.

I see two viable solutions:

• Keep things as they are and advice developers to change rollup.config.js on their local system if they don't want the replacement.
• Introduce a new rollup config rollup.dev.config.js for npm start/npm run dev. This config file would not contain glconstants().

[takahirox]

Yeah we now use three.module.js in examples and production so may be better to go with either one of your suggestions. (By the way off topic, why we don't have three.module.min.js? Minified module doesn't make sense?)

[Mugen87]

By the way off topic, why we don't have three.module.min.js?

#15176 😉

[takahirox]

Thanks. We may need to revisit if three.module.min.js lands but I think it's good enough to advice(document) or have a new roolup config for now.

[Mugen87]

I'm with both approaches fine, too. Let's see what @mrdoob prefers.

[mrdoob]

Yes, I think it's we should try to add a three.module.min.js.
And we should only convert the gl constants in the min files 👌

[Mugen87]

After reconsidering this issue I think we are overlooking an important aspect.

Most serious apps work with the three npm package and use a build tool to create the final app bundle. Since the replacement of gl constant should improve performance, you really want this optimization in three.module.js.

I've pointed out here why I think adding three.module.min.js is a bad idea so three.js would be the only build file where no constants are replaced. Since this artifact has lost importance due to the module migration, I don't vote to make this change anymore. I think it's best if devs manually edit rollup.config.js on their system if they want no constant replacement.