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

chore(nuxt-module): add critical packages to module's dependencies #531

Merged
merged 18 commits into from
May 24, 2024
Merged

chore(nuxt-module): add critical packages to module's dependencies #531

merged 18 commits into from
May 24, 2024

Conversation

MuhammadM1998
Copy link
Contributor

@MuhammadM1998 MuhammadM1998 commented May 6, 2024

Resolves #529

This adds the following packages as dependencies so that the user don't need to install them.

  • class-variance-authority
  • clsx
  • radix-vue
  • tailwind-merge
  • tailwindcss-animate

This means a cleaner package.json, I've updated the playground accordingly

"dependencies": {
-  "class-variance-authority": "^0.7.0",
-  "clsx": "^2.1.1",
-  "radix-vue": "^1.7.3",
-  "tailwindcss-animate": "^1.0.7"
-  "tailwindcss-merge": "^2.0.3",
  "shadcn-nuxt": "^0.8.0"
}

Note that clsx is already a dependency in class-variance-authority and we're using the same major version v2 so it can also be safely removed

Tested locally in the playground & a separate project and it works as expected. run pnpm why radix-vue and you'll see a similar dependency tree
Screenshot 2024-05-06 111853

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented May 6, 2024

Since it has same changes from this PR #528 but with +3 new commits please add some text like

Note

This PR depends on #528 to get merged first

@MuhammadM1998
Copy link
Contributor Author

@sadeghbarati Didn't realize it until I made the PR sorry for that😅 I modified it to include only the changes for this PR

@sadeghbarati
Copy link
Collaborator

@MuhammadM1998, another point regarding the shadcn-vue CLI init command, when we execute it in Nuxt projects, it still installs those dependencies. Therefore, ensure to update the cli package too

@MuhammadM1998
Copy link
Contributor Author

Oh you're right will push changes later today

@MuhammadM1998
Copy link
Contributor Author

@sadeghbarati Refactored the CLI to check if shadcn-nuxt is installed and checks its version to ensure that it handles the dependencies for backward compat.

@sadeghbarati
Copy link
Collaborator

Let's wait for Zernonia's opinion cause he is the author of this module 💯

@sadeghbarati sadeghbarati requested a review from zernonia May 9, 2024 07:40
@MuhammadM1998 MuhammadM1998 changed the title chore: add critical packages to module's dependencies chore(nuxt-module): add critical packages to module's dependencies May 10, 2024
@zernonia
Copy link
Member

Thanks for the PR @MuhammadM1998 ..

One question tho, if user decide to use other version of Radix Vue, how can they install a different version of it?

@MuhammadM1998
Copy link
Contributor Author

@zernonia Thanks! Users can use overrides to use a different version.

"dependencies": {
  "shadcn-nuxt": "^3.0.0", // Has radix-vue v1.7.3 as a dependency
},
"overrides": {
  "radix-vue": "^2.0.0", // This version is what will be installed
}

@sadeghbarati
Copy link
Collaborator

@MuhammadM1998 Let's remove radix-vue from shadcn-nuxt module deps?

Most of the users are unaware of overrides module features in packageManagers

@sadeghbarati
Copy link
Collaborator

Also, let's consider checking this Nuxt PR too: nuxt/nuxt#27155

If it gets merged we don't need @oxc-parser/wasm anymore I think

@MuhammadM1998
Copy link
Contributor Author

MuhammadM1998 commented May 21, 2024

@MuhammadM1998 Let's remove radix-vue from shadcn-nuxt module deps?

Most of the users are unaware of overrides module features in packageManagers

Normally a user won't need to use overrides. A case where a user would need that is that shadcn-nuxt bumped radix-vue to 1.7.8 for example and it caused problems for the user so then he'd use overrides to install radix-vue 1.7.7. That's a slim chance as you see. I'm okay with removing it from dependencies (maybe move it to peerDeps?) but imo its better this way. I'll wait for you decision. Feel free to push changes too!

@sadeghbarati
Copy link
Collaborator

@MuhammadM1998 Let's merge it for now for further PRs 🙏

@sadeghbarati sadeghbarati merged commit bd0a820 into unovue:dev May 24, 2024
1 check passed
@MuhammadM1998 MuhammadM1998 deleted the chore/module-deps branch May 24, 2024 13:52
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.

Feature(Nuxt): Add radix-vue and other main deps as dependencies
3 participants