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

Do not rely on THREE global #13

Open
arjanfrans opened this issue Dec 20, 2016 · 12 comments
Open

Do not rely on THREE global #13

arjanfrans opened this issue Dec 20, 2016 · 12 comments

Comments

@arjanfrans
Copy link

I am building my application using Webpack and started using ES6 modules to import three.js functions. Since this module relies on the THREE global, I ran into a problem that it is undefined inside this module.

It works for now by setting the global THREE manually. However, I think it would be nice to add three.js as a peer dependency on this module and import the required function from three.js inside, instead of relying on the global.

What do you think?

@mattdesl
Copy link
Contributor

mattdesl commented Dec 20, 2016

This already has a peer dependency on ThreeJS, it's just not explicit in the package.json. 😄 This means you can use any build of ThreeJS (i.e. with browserify/webpack, or just a plain <script> tag before your bundle) as long as it's in the global scope.

Explicitly requiring the three package will cause a lot of issues. Many production-scale projects ship with their own versions of ThreeJS rather than the three module. Also, because there are so many different versions of ThreeJS, it's very likely that you will end up with multiple instances in the same bundle if your code depends on a module that depends on some mismatched version of ThreeJS.

Global scope isn't ideal but I'm not sure there is any solid solution yet. See more here:
mattdesl/three-orbit-controls#17 (comment)
https://gist.github.com/mattdesl/06d056f10322532e6e07bc97ef91b557

@iangilman
Copy link

I ran across this issue as well, using webpack, and I had to dig around to find the webpack command to make it work. I'm posting it here in case other people had the same problem. In your webpack.config.js configuration object, add:

  plugins: [
    new webpack.ProvidePlugin({
      THREE: 'three'
    })
  ]

@mflux
Copy link

mflux commented Apr 17, 2017

Running into this issue as well. We have a build that uses webpack and we're loading pre-bundled three from a lib folder because it's a modified three.js build. Including bmfont-text via npm will cause THREE to not be found, both using global or window object because it's all loaded at once in the bundle. Even using ProvidePlugin solution above does not work since THREE isn't installed via NPM.

This was actually a huge headache for datguivr as well. We originally wanted to do tree shaking via rollup but this module couldn't find THREE either.

Not exactly sure what the right solution for this lib is though :| It probably should include THREE ES6 module bits.

The problem stems from immediately needing THREE to be in scope at

var Base = THREE.BufferGeometry

This happens as soon as an import statement from bundlers hit BMFontText. You literally can't declare window.THREE = THREE or global.THREE = THREE before this happens unless it takes place in a separate script.

@iangilman
Copy link

Indeed... seems like one solution would be to refactor this library to not alias THREE until it's first called. That wouldn't change the API for anyone else, but would allow for workarounds for scenarios like @mflux has.

@jamieowen
Copy link

jamieowen commented Jul 28, 2017

I made these quick changes to get this library working with ES6 / Browserify.

jamieowen@10e3ecc

It's not really PR ready as i had to pull in some other deps code directly, but just thought i'd reference it here..
But i've had similar nightmares with ES6 and duplicate versions of three.js.

The cleanest and safest way i've discovered so far for ES6 use for custom three modules is to reference three.js only as a proper peerDependency in the package.json. Similar to this ( https://github.com/jamieowen/three-material-modifier/blob/master/package.json )

But definitely don't reference three in dev or prod dependencies ( even for demo-making purposes ) as a version still gets put in the deps node_modules.

Also you need to add NODE_PATH=./node_modules before your browserify/budo related commands otherwise node will complain about not finding three in your deps package.

NODE_PATH=./node_modules budo src/index.js

I've had some weird shit happen when other dependencies include three.js, like declaring reusable Geometry in the top level scope of a js file causes some weird rendering problems that are basically impossible to debug as there are no errors, just nothing appears but only in certain circumstances.. confusing as hell! :)

I haven't figured out why those problems happen exactly but this setup has really helped me avoid any issues.

@AlbertoElias
Copy link

This does make it difficult to use with ES2015 modules though, as THREE needs to be set as a global, as a hack, somewhere before this module is imported, but due to the dependency graph, it's not that easy to do properly

@pablokvitca
Copy link

Has anyone gotten the library working inside Angular 5? Declaring THREE as a global variable (window.THREE = THREE or document.THREE = THREE) don't seem to do anything. I keep getting an error complaining THREE is not defined. I think a simple way to allow developers to make the library use whatever version they want of three.js is for the THREE object to be passed as an argument when calling the library. This approach would allow maintaining the flexibility of the library while also supporting modules on ES2015 and frameworks such as Angular and others.

@dbuck
Copy link

dbuck commented Sep 24, 2019

I went the slightly different and hackier route. I don't want to add ALL of 'three' to our bundle size (we've got some stripping going on), but we can forward the parts needed into the global three to allow the package to work without modifications.

// three-bmfont-shim.ts
import { BufferGeometry, Sphere, Box3, Texture, Color, BufferAttribute } from 'three';
(window as any).THREE = { ...(window as any).THREE, BufferGeometry, Sphere, Box3, Texture, Color, BufferAttribute };

@stur86
Copy link

stur86 commented Jan 6, 2021

I agree this is a pretty bad decision. There are hacks to go around it but that's all they are, hacks. I don't see what's the problem with importing THREE in the library's own file locally, since anyway that will default to whatever the version any given user has installed in node_modules is.

@wilson0x4d
Copy link

this could be better.

@mattdesl
Copy link
Contributor

mattdesl commented Feb 9, 2021

🤷‍♂️

ThreeJS has come a long way in terms of npm/modularity/build tools since this issue was opened (in 2016), so it makes sense to re-evaluate, and that discussion has already started in a PR here. But it's not really as simple as just accepting the PR as it stands now and making a breaking change, as it will likely break a lot of the software that is using this module in production.

For example, AFrame is using this module, and (no surprise) they have forked ThreeJS and are using their own version in their library rather than relying on node_modules. So if I do include require('three') in this module, then any user installing AFrame would end up with two versions of ThreeJS in node_modules. Forking ThreeJS like this is really common in larger productions. I do agree the best thing for the future is to depend directly on three module and just let the dependents figure it out themselves (for example aliasing three imports to their own fork), but it's not really a decision I take lightly and I don't see any urgency to it since both choices here lead to headache and more issues in my inbox, and I also have other things I'd rather be doing with my time than worrying about whether or not THREE is in global scope in this module. An even better solution would be to pass this torch to a team that has a more vested interest in the longevity of this module (such as somebody from AFrame), but a lot of that team disappeared when Mozilla had their layoffs.

So yeah; the decision may appear poor now in hindsight but it's also more nuanced than it might appear, and times have change since 2016.

🍻

p.s. @wilson0x4d Commenting "this could be better" is a waste of my time as it goes to my inbox as an email. Maybe just fork this module and move on, or don't use my module at all if you don't agree with it.

@stur86
Copy link

stur86 commented Feb 9, 2021

To me it sounds like it would be on whoever is curating that library to then take care that, if they fork THREE, they also fork modules like this one and make the necessary changes. It leaves me puzzled that this is the one case that controls the decision rather than the most straightforward one where I npm install THREE, then this module, and expect them to work out of the box like one would think they ought to. But I understand that at this point it might be problematic to just backtrack, as many of those libraries would just end up not being fixed because the maintainers might never realise what's changed, etc. Either way, personally I actually ended up forking this module into my own version just for this.

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

No branches or pull requests

10 participants