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 controls import #5

Merged
merged 1 commit into from
May 1, 2020
Merged

update controls import #5

merged 1 commit into from
May 1, 2020

Conversation

RaulPROP
Copy link
Contributor

I have updated the controls so the users can access new features as in this issue.

I also added a .gitignore file.

import FlyControlsWrapper from 'three-fly-controls';
const ThreeFlyControls = (FlyControlsWrapper(three), three.FlyControls);
import { TrackballControls as ThreeTrackballControls } from 'three/examples/jsm/controls/TrackballControls.js';
import { OrbitControls as ThreeOrbitControls } from 'three/examples/jsm/controls/OrbitControls.js';
Copy link

@sirrodgepodge sirrodgepodge Apr 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't you just import from three? not the example code? The actual three dependency is up to date btw (I checked that when trying to debug)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to, but node couldn't find the object. The only part of the code where the controls are exported are on the example folder.

Also I did a fast research and found that on the threejs examples they export them from that path.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow looks like it, opened an issue here: mrdoob/three.js#19250

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay it actually looks like this is the approved way to do it:
mrdoob/three.js#19250 (comment)

@vasturiano
Copy link
Owner

Thanks @RaulPROP for this. I think it would be certainly a good shift to consume the controls directly from the three package.

However this is a bit of a problematic approach, because three is not a dependency of this module, but a peerDependency. That means it's not guaranteed to be available for import at bundle time and is expected to be provided by the consumer.

In the UMD version this takes the form of the consumer including a script tag which exposes a global understood by the bundler (rollup).

Since the controls are not exposed as part of the core three build, things gets a bit complicated. We could either:

  1. Shift three to a regular dependency instead of peerDependency. This is highly undesirable as it would over bloat the module bundle size. It would also prevent the consumer from having a single instance of three in the application, since it's very likely this lib will be used side by side with other three functionality.

  2. Expect the UMD consumers to include additional script tags for all the controls used in the module, and register all of those as expected globals in rollup. This requirement would be confusing for the users imo, and would lead to quite a bit of head-scratching and unexpected hard to debug linking issues, because the need for it is not really apparent.

  3. Include three as devDependency in addition to peerDependency. This is probably the most promising of all the options, but it is kind of hackish as that's not really the main use of devDependencies. And I'm also not 100% sure is not gonna lead into dependency conflicts upstream.

I'm inclined to give 3) a try, unless you can think of a better way to approach it.

Hopefully it's clear why this is a less straight-forward change than it immediately appears. This is the reason why I initially went the route of importing the controls from separate packages, because you can then separate the peer from the regular dependencies.

@RaulPROP
Copy link
Contributor Author

Oh, I didn't notice this problem.

I don't know which one is the best solution, I don't have much experience dealing with this situations.

Maybe we could create a fork of the libraries that this package is currently using and update the code to match the three.js/examples ones?

@vasturiano vasturiano merged commit d6a4b58 into vasturiano:master May 1, 2020
@vasturiano
Copy link
Owner

Ok I went with option 3). Also removed the stand-alone dependencies since they're no longer used.

Thanks!

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.

3 participants