Bug when using mathbox + THREE.Color in a bundle

[ChristopherChudzicki]

@sritchie brought this up in https://github.com/ChristopherChudzicki/mathbox-react/pull/203, but fundamentally it's an issue with the way Mathbox imports stuff.

Expectation: Users consuming the mathbox library should be able to write code similar to that below, bundle it with some bundler (webpack, vite, turbopack, etc) and have it work

import * as MB from "mathbox"
import * as THREE from "three"

const mathbox = MB.mathBox(/* options... */)
mathbox
  .cartesian()
  .axis({ color: new THREE.Color(0xff4136) })

Actuality: Code like the above does not work because:

1. This library (mathbox) imports from three/src. For example, import { Color } from "three/src/math/Color.js"; ( example)
2. This library (mathbox) uses instanceof checks like value instanceof Color (example)
3. Userland people like above import from three (such as the code above), which means userland people use three/build/three.module.js. Consequently, instanceof checks won't work.

See https://github.com/ChristopherChudzicki/mathbox-color-bug for a full example.

Notes

• This doesn't just affect color. This probably affects any option that is passed as an object created from THREE; https://github.com/unconed/mathbox/blob/master/src/primitives/types/types.js includes a bunch of other instanceof checks.
• This does not affect users including mathbox via CDN, I believe because of the externals aliasing done here https://github.com/unconed/mathbox/blob/master/config/webpack.config.js#L16

[sritchie]

I looked into this a bit (while dealing with some madness around an exploded frozen pipe at our house, otherwise I would put up a full PR...)

It looks like internally, threejs deals with this with methods like isColor: https://github.com/mrdoob/three.js/blob/dev/src/math/Color.js#L59

isVector2: https://github.com/mrdoob/three.js/blob/dev/src/math/Vector2.js#L5

etc... so my guess is that that is the best way for us to move forward here. Thoughts @ChristopherChudzicki ?

[ChristopherChudzicki]

Submitting this in haste b/c i have to run... hopefully didn't forget something.

I would start off with the question "Do we want to support userland imports like import ... from "three". I think the answer is definitively "Yes". We could tell userland people "You should import from "three/src". But that feels (a) less natural, and (b) is potentially troublesome for builds [^1], and (c) seems fragile. Regarding (c)... importing their src files really doesn't seem like expected to usage to me. They do include "src" in their package.js exports + files lists, but their src files really seem like more an implementation detail to me: would they really consider renaming a src file a breaking change?

[^1]: Using import { Color } from "three/src/Three.js" works fine the minimal example in mathbox-color-bug. But it did not work in the storybook example at https://github.com/ChristopherChudzicki/mathbox-react/pull/203. Importing from "three/src" crashed storybook for some reason I never fully figured out.

OK, why did we start importing from "three/src" in the first place? Answer: to make mathbox easier for bundlers to tree-shake. Note!: Whether we do import { Color } from "three" or import { Color } from "three/src" does not affect mathbox's bundle size since those are external/peerDependencies, but the difference can affect how easy it is for userland apps (like math3d-next or your projects) to tree-shake.

Suggestion

My suggestion: Stop importing from "three/src". Replace all our threejs imports with import { Color, Whatever } from "three".

Pros:

• we use their built code, which seems more definitively their public api
• it's still tree-shakeable. See https://github.com/ChristopherChudzicki/bundling-tests for a proof of concept.

Cons:

• Although some bundlers can still treeshake the unused portions of ThreeJS, webpack seems unable to. I'm not sure if it's really unable to, or if I just haven't gotten the config correct[^2].

[^2]: I probably won't try much longer. Webpack is frustrating. Thus I switched to vite (for math3d-next) and rollup (for mathbox-react).

@sritchie Question: Left to my own devices, I would switch to "three" imports and tell people to use vite/rollup/whatever instead of webpack if they want better treeshaking. But I'm unsure how important webpack is to you + clojurescript ecosystem.

[ChristopherChudzicki]

@sritchie Re using properties like isVector2 or isColor: I think this could work and I'm OK with that if the the above suggestion—using import ... from "three"—seems like a no-go to you. It occurs to me that this would mean that if I import { Color } from "three" and use a bundler that can successfully tree-shake such imports, then I still end up with two copies of Color: one from my userland import and one from mathbox. Of course, we could tell userland people to use three/src/ imports, but I don't love that for reason above.

[ChristopherChudzicki]

Related:

• https://github.com/mrdoob/three.js/issues/24199

Though I haven't found anything particularly enlightening yet.

[sritchie]

@ChristopherChudzicki thanks for doing all of this research. Let me ask the resident Google Closure Compiler expert on the Clojure slack if he thinks that my switch over to the three/src imports is actually doing anything here, of if Closure can successfully tree-shake from "three" imports. I'll get this done in the next day or so!

[sritchie]

@ChristopherChudzicki based on that tree-shaking thread I'm convinced that I don't have a good mental model of how any of this works, and I am very happy to switch to "three" imports.

THAT SAID — using the isColor etc methods might also be a good change, because if some consuming project decides that THEY want to import from just the file, their code will break for the opposite reason that my example broke.

It seems that threejs has added these to every object where we are using instanceof checks, and dodging instanceof completely feels like a good change.

wdyt @ChristopherChudzicki ? is this more work for not much gain?

[ChristopherChudzicki]

@sritchie 👍 , I agree; I am inclined to do both:

• simplify imports to "three" ... library end users can use simple "three" imports, and some bundlers can still tree shake sufficiently well. If your bundler can't, that's a bundler problem.
• use isColor to check if object is color, isVector2 for vector2, etc, for the reason you mention