Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Multiple instances of Three.js being imported" from CDN #21167

Closed
hmans opened this issue Jan 28, 2021 · 8 comments · Fixed by #21258
Closed

"Multiple instances of Three.js being imported" from CDN #21167

hmans opened this issue Jan 28, 2021 · 8 comments · Fixed by #21258

Comments

@hmans
Copy link

hmans commented Jan 28, 2021

Describe the bug

Three r125 now checks if multiple different instances of the library are being imported. Weirdly, though, this is already triggered simply by following the "Installation" section of the documentation:

<script type="module">
  import * as THREE from "https://unpkg.com/three/build/three.module.js";
  import { OrbitControls } from "https://unpkg.com/three/examples/jsm/controls/OrbitControls.js";
  /* Warning triggered */
</script>

Here's a Codesandbox with the issue:
https://codesandbox.io/s/three-multiple-instances-warning-nkfc9

I've tried a bunch of other ESM CDNs (skypack and jspm, specifically), with the same results.

To Reproduce

  1. Visit the codesandbox
  2. Check the console output

Code

See above.

Live example

Expected behavior

There should not be a warning that multiple instances are being imported if they're being imported via JS modules from CDNs.

Platform:

  • Three.js version: [r125]
@donmccurdy
Copy link
Collaborator

Hmm, the warning is correct. From the network tab, with cache enabled:

Screen Shot 2021-01-28 at 12 00 46 PM

Note that unpkg redirects unpkg.com/three to unpkg.com/three@0.125.1. It seems like even though the two references to three.module.js resolve to the same file, the browser is importing it twice. This does not happen if we use the versioned CDN URL instead, avoiding the redirect:

import * as THREE from "https://unpkg.com/three@0.125.1/build/three.module.js";
import { OrbitControls } from "https://unpkg.com/three@0.125.1/examples/jsm/controls/OrbitControls.js";
/* Warning NOT triggered */

Seems like we need to ensure the documentation uses version-pinned URLs, then?

@hmans
Copy link
Author

hmans commented Jan 28, 2021

Yeah, that fixes it.

It does leave me with the problem that once I add another (ESM) library on top that imports from "three", the warning returns. As long as we don't have import maps, there is no way I can tell the library to import from that specific, version-pinned URL.

I understand why this is happening -- I think -- but what weirds me out is that these things were fine in r124? Or did they just seem fine because we didn't have that warning then?

@hmans
Copy link
Author

hmans commented Jan 28, 2021

Update: example sandbox here for my previous comment.

@donmccurdy
Copy link
Collaborator

I understand why this is happening -- I think -- but what weirds me out is that these things were fine in r124? Or did they just seem fine because we didn't have that warning then?

I don't think r125 changed anything about module structure, other than detecting and warning about this issue. So, yeah, the issue just got missed before.

Importing three.js twice isn't necessarily going to cause obvious issues, but it can definitely cause some subtle ones. For example mesh instanceof Mesh won't work. We very rarely use that pattern in three.js (we use mesh.isMesh instead) but other libraries might.

It does leave me with the problem that once I add another (ESM) library on top that imports from "three", the warning returns

At some point, an application with intertwined dependencies can't really use a normal CDN to load packages. Maybe Skypack is the exception to that, since it handles resolution cleverly, but I haven't tested it to say more. three.js goes to a lot of trouble to make this case mostly just work, but most npm packages don't, and in all fairness it's difficult and messy.

Since you're the author of three-elements, one thing you could do would be to make three.js a peer dependency and compile it as "external" in your non-ES-Module builds. See the build process for three-pathfinding for an example of that:

https://github.com/donmccurdy/three-pathfinding/blob/8943c5818050045a27a1334327355f3c5d1f3763/package.json#L21

@hmans
Copy link
Author

hmans commented Jan 28, 2021

Since you're the author of three-elements, one thing you could do would be to make three.js a peer dependency and compile it as "external" in your non-ES-Module builds. See the build process for three-pathfinding for an example of that:

I believe I'm already doing that, but I'll give it another look to see what I can tweak tomorrow. But just to make sure I understand you correctly, this will only help when loading the non-ESM version, with window.THREE defined through a separate UMD import of Three.js, correct? (ie. I won't be able to use <script type="module">?)

@donmccurdy
Copy link
Collaborator

Ah, you might be right yes. I believe three-pathfinding only supports traditional CDNs when using the UMD builds, not ES Modules. I'm considering the three-pathfinding ES Module builds to be meant for bundlers, and perhaps Skypack CDN.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 11, 2021

I'm unsure about this issue. It seems to me the mentioned problem is a general JavaScript thing. Or is there a concrete suggestion for a change in three.js?

@hmans
Copy link
Author

hmans commented Feb 11, 2021

I'm unsure about this issue. It seems to me the mentioned problem is a general JavaScript thing. Or is there a concrete suggestion for a change in three.js?

The bug is that this happens when following the official instructions on the website. As @donmccurdy already pointed out, it could be argued that this is a bug with the documentation, not the library itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@hmans @donmccurdy @Mugen87 and others