-
Notifications
You must be signed in to change notification settings - Fork 174
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
Update index.js #17
base: master
Are you sure you want to change the base?
Update index.js #17
Conversation
Thanks for the PR! This is actually a design decision, see here for details: I'm open to some more discussion around it, though. 😄 |
Having Three as a global doesn't work nicely with ES2015 modules and rollup. I have Three as a module too, and I manually add it to the global scope as soon as I can, but all other module code is executed beforehand, including this module, so it breaks as THREE is still undefined. This would be fixed if the Base variable is instantiated in some function, maybe the exported one, so it's not executed at runtime |
So, almost 3 years have passed since this reply. Is this still your position @mattdesl ? I personally would like to be able to tree shake my build code and relying on the global scope though covers your use case described above, does not help to keep build sizes small. So wonder where your thoughts are a couple of years after this PR and now that three is starting to get some traction behind tree shaking (not yet working as per this comment). |
I think its more reasonable now to rely on I haven't done much development on this module, since it is being used in AFrame perhaps those folks would like to have access/ownership to it? |
Fair enough. I'm only now having a better look at it and how you implemented the whole thing. Its used throughout most corners of the internet so would be lovely to modernise this a little bit. I understand your points and concerns over A-Frame and other applications that might be requiring it and I'll wait until anyone picks this up. However if you could tag someone from their team and see if there's interest, I would happily chase @kig and @Samsy as they both had good suggestions for upgrading this module. Thanks for your reply! |
See my pull request #39, THREE can be included in a way that's compatible with browser and commonjs module. |
So module no longer relies on global THREE variable