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

Update index.js #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

wiseshrimp
Copy link

So module no longer relies on global THREE variable

@mattdesl
Copy link
Contributor

Thanks for the PR!

This is actually a design decision, see here for details:
#9 (comment)

I'm open to some more discussion around it, though. 😄

@AlbertoElias
Copy link

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

@andrevenancio
Copy link

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

@mattdesl
Copy link
Contributor

I think its more reasonable now to rely on require('three') and use a peer dependency here. It would be a major breaking change and would potentially affect many users of this module (most notably A-Frame which I believe is using it for their text rendering), so its worth pinging some of those people to let them know.

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?

@andrevenancio
Copy link

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!

@Alchemist0823
Copy link

See my pull request #39, THREE can be included in a way that's compatible with browser and commonjs module.

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

Successfully merging this pull request may close these issues.

5 participants