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

[Bug]: @floating-ui/* dependencies is both added to bundle and installed in client's node_modules #443

Closed
floroz opened this issue Oct 4, 2023 · 3 comments · Fixed by #436
Assignees
Labels
bug Something isn't working

Comments

@floroz
Copy link

floroz commented Oct 4, 2023

Environment

N/A

Link to minimal reproduction

N/A

Steps to reproduce

N/A

Describe the bug

I was looking at the build setup of the library and I noticed that @floating-ui/vue and @floating-ui/dom are shipped as dependencies in package.json.

This means that when client are going to install radix-vue they will also install a copy of those two libraries in their node_modules.

At the same time, in the vite.config.ts, the only build.rollupOptions.external dependency declared is vue.

This means, that the library also does bundle the functions from floating-ui directly into the index.js.

Is my understanding correct?

Expected behavior

Either

  • declare @floating-ui/* to external to avoid bundling the references

or

  • move @floating-ui/* from dependencies to devDependencies to avoid client to having to install it in their node_modules.

Conext & Screenshots (if applicable)

No response

@floroz floroz added the bug Something isn't working label Oct 4, 2023
@floroz floroz changed the title [Bug]: floating-ui dependency is both bundled and installed from the client's side [Bug]: @floating-ui/* dependencies is both bundled and installed from the client's side Oct 4, 2023
@floroz floroz changed the title [Bug]: @floating-ui/* dependencies is both bundled and installed from the client's side [Bug]: @floating-ui/* dependencies is both added to bundle and installed from in client's node_modules Oct 4, 2023
@floroz floroz changed the title [Bug]: @floating-ui/* dependencies is both added to bundle and installed from in client's node_modules [Bug]: @floating-ui/* dependencies is both added to bundle and installed in client's node_modules Oct 4, 2023
@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Oct 4, 2023

It could1 or should be dependencies but it must mark as external in vite.config

Also vue and @vueuse/core should be in peerDependencies

since Vue support (simple) external types from ^3.3.0

Footnotes

  1. floating-ui also can be in peerDependencies too

@floroz
Copy link
Author

floroz commented Oct 4, 2023

It could1 or should be dependencies but it must mark as external in vite.config

Also vue and @vueuse/core should be in peerDependencies

since Vue support (simple) external types from ^3.3.0

Footnotes

  1. floating-ui also can be in peerDependencies too

@sadeghbarati adding @vueuse/core to peerDependencies would have the trade-off of locking the end users to that version range (if @vueuse/core bumps their major, and clients want to upgrade, we would need to hope it doesn't break the existing API used by radix-vue).

I'm not too familiar with the release cadence of that package and the lifecycle of their major versions.

What do you think?

My team is building a large component library and we're facing similar challenges and decisions, so we're trying to gain a broader perspective on some of the best practices around the vue ecosystem of third-party libraries.

@zernonia
Copy link
Member

zernonia commented Oct 5, 2023

Hey @floroz ! We've included @floating-ui as a dependencies as many of our components depend on it, and we would like to stick with the targeted version to make sure radix-vue's component work as expected.

You are right about setting @floating-ui/vue, @floating-ui/dom as external, and output.globals! It shouldn't be bundled into index.js.

@zernonia zernonia self-assigned this Oct 23, 2023
@zernonia zernonia linked a pull request Oct 23, 2023 that will close this issue
Merged
5 tasks
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

Successfully merging a pull request may close this issue.

3 participants