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

refactor: use get-tsconfig instead of tsconfig-paths #880

Closed
wants to merge 3 commits into from

Conversation

sadeghbarati
Copy link
Collaborator

@sadeghbarati sadeghbarati commented Nov 10, 2024

πŸ”— Linked issue

Close #291
Close #806
Close #560
Close #559
Close #430 (not sure about WSL, but it must be fixed)

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

#291 (comment)

Please feel free to push new changes

Also, I did not update the test files

export async function getRawConfig(cwd: string): Promise<RawConfig | null> {
try {
const configResult = await c12LoadConfig({
name: 'components',
configFile: 'components.json',
configFile: 'components',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed again to components so user could use any supported format in unjs/c12 for components.json/json5/... file

@@ -139,7 +139,7 @@ export async function promptForConfig(
message: (prev, values) => `Where is your ${highlight(values.typescript ? 'tsconfig.json' : 'jsconfig.json')} file?`,
initial: (prev, values) => {
const prefix = values.framework === 'nuxt' ? '.nuxt/' : './'
const path = values.typescript ? 'tsconfig.json' : 'jsconfig.json'
const path = values.framework === 'nuxt' ? 'tsconfig.json' : values.typescript ? 'tsconfig.json' : 'jsconfig.json'
Copy link
Collaborator Author

@sadeghbarati sadeghbarati Nov 10, 2024

Choose a reason for hiding this comment

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

If the framework is Nuxt, CLI initial value must be .nuxt/tsconfig.json

But the current CLI is using .nuxt/jsconfig.json in JS projects

Copy link

Choose a reason for hiding this comment

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

What is the reason of using .nuxt/tsconfig.json instead of base ./tsconfig.json for a Nuxt project? If get-tsconfig correctly resolves extends, then the result will be the same, right?

The advantage of using base ./tsconfig.json could be in respecting any other custom options user provided in this file. But I'm not sure how exactly tsconfig is used by shadcn-vue except the path resolving, so maybe I'm wrong.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah you are right, we can simplify the prefix and path variables

I did not test with base ./tsconfig.json in root of the Nuxt project but it should work. maybe with both

  1. ./tsconfig.json or ./jsconfig.json
{
 // https://nuxt.com/docs/guide/concepts/typescript
 "extends": "./.nuxt/tsconfig.json"
}
  1. .nuxt/tsconfig.json

@sadeghbarati
Copy link
Collaborator Author

sadeghbarati commented Dec 4, 2024

This PR will be included in the Next PR #917

So closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants