Bug when using mathbox + THREE.Color in a bundle about mathbox HOT 7 CLOSED

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 ?

from mathbox.

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 ¹, 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?

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².

@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.

from mathbox.

@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.

from mathbox.

Related:

• mrdoob/three.js#24199

Though I haven't found anything particularly enlightening yet.

from mathbox.

@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!

from mathbox.

@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?

from mathbox.

@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

from mathbox.