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

Please consider moving troika-three-text to peerDependency #74

Open
alexzhang1030 opened this issue Nov 19, 2024 · 7 comments
Open

Please consider moving troika-three-text to peerDependency #74

alexzhang1030 opened this issue Nov 19, 2024 · 7 comments
Labels
bug Something isn't working

Comments

@alexzhang1030
Copy link
Contributor

  • three version: r170
  • @pmndrs/vanilla version: latest
  • node version: N/A
  • npm (or yarn) version: N/A

Problem description:

This is a reproduction link:

https://stackblitz.com/edit/vitejs-vite-vmldb2?file=package.json

Now we use troika-three-text 0.47.2 as a dependency, meaning we cannot upgrade this dep. But if this version isn't compatible with a higher Threejs version, it will throw an error and cannot be fixed.

I developed Text in #37 and make this dep to peerDep, but sadly @vis-prime has moved it to dep in #44.

@alexzhang1030 alexzhang1030 added the bug Something isn't working label Nov 19, 2024
@vis-prime
Copy link
Collaborator

vis-prime commented Nov 19, 2024

Better to remove it then , others have also reported issues with this
If its kept in peer dep it leads to other issues as the user might not have troika installed or versions might not match

In the readme will post a link to https://www.npmjs.com/package/troika-three-text as its already runs with vanilla-js

@vis-prime
Copy link
Collaborator

vis-prime commented Nov 19, 2024

removed it for now for testing, if its absolutely needed ,will add it back

text is removed in [email protected]
this lib can now be used in bundless environments https://jsfiddle.net/f7o0zsrg/15/

@alexzhang1030
Copy link
Contributor Author

alexzhang1030 commented Nov 20, 2024

I think you misunderstood my issue🤣

You can move troika-three-text to a peer dependency (not removing Text), and then I can use the Text of @pmndrs/vanilla and a higher version of this troika-three-text in my project. That could work perfectly with Threejs r170.

@vis-prime
Copy link
Collaborator

vis-prime commented Nov 20, 2024

If it's in peer deps it causes installation error if the user does not have troika text installed

They are forced to install troika text even if they don't use the 'text' element

And in bundless environment also troika will be needed separately (on discord some users mentioned this issue)

Better to keep use it as a external lib since it's already vanilla js (text removal was planned long back when the issues poped up, but had forgotten about it 😭)

@alexzhang1030
Copy link
Contributor Author

If it's in peer deps it causes installation error if the user does not have troika text installed

They are forced to install troika text even if they don't use the 'text' element

And in bundless environment also troika will be needed separately (on discord some users mentioned this issue)

Better to keep use it as a external lib since it's already vanilla js (text removal was planned long back when the issues poped up, but had forgotten about it 😭)

We can configure the dep as an optional peer dep by this https://docs.npmjs.com/cli/v7/configuring-npm/package-json#peerdependenciesmeta

@alexzhang1030
Copy link
Contributor Author

But that's ok if you decide to remove Text permanently.

@vis-prime
Copy link
Collaborator

If decision is made to bring text back , will test the peer dep and optional Boolean 👍

for now lets advise users to use troika text directly, without the additional wrapper

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants