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

[Feature]: Move InjectionKey export from FormItem.vue to .ts file to avoid warnings #176

Closed
2 tasks done
olemarius opened this issue Nov 17, 2023 · 12 comments
Closed
2 tasks done

Comments

@olemarius
Copy link
Contributor

Describe the feature

Today useFormField.ts imports FORM_ITEM_INJECTION_KEY from FormItem.vue.

In a monorepo with a /package/my-lib with shadcn-vue set up, this gives the following error when used in an app in the same monorepo:

image

I've added the 'import/first': 'off' rule both in the library's eslint config and the app importing the library's eslint config with no luck. My local fix for this is to move the InjectionKey out to a separate .ts file and import it from there.

/ui/form/InjectionKey.ts:

import { type InjectionKey } from 'vue'

export const FORM_ITEM_INJECTION_KEY
  = Symbol() as InjectionKey<string>

I'm not sure why I can't import from vue files, but I think using a .ts file might be a better approach anyways.

Let me know if I should create a PR.

Additional information

  • I intend to submit a PR for this feature.
  • I have already implemented and/or tested this feature.
@sadeghbarati
Copy link
Collaborator

Are you using Nuxt or Vite/Vue?

@zernonia
Copy link
Member

Oh yeah 1 thing I forgot to check with you @olemarius . Can you share the component you are using these Form component that throws this warning? 😁

@olemarius
Copy link
Contributor Author

It's a monorepo using pnpm and the app importing bloc-ui which uses shadcn-vue is a vite + vue 3 + typescript app.

My setup looks like this

/root (turborepo/pnpm)
   - /apps/
       - /bloc/ => imports bloc-ui
   - /packages
       -/bloc-ui/ => shadcn-vue here

Here's the typical anatomy

<FormColumn>
    <FormField
        v-slot="{ componentField }"
        name="title">
        <FormItem>
            <FormLabel data-cy="blog-title-label">
                Title
            </FormLabel>
            <FormControl>
                <Input
                    data-cy="blog-title"
                    v-bind="componentField"
                    name="title"
                    type="text" />
            </FormControl>
            <FormDescription>
                Choose a describing title
            </FormDescription>
            <FormMessage data-cy="blog-title-message" />
        </FormItem>
    </FormField> 
</FormColumn>

If you want I can test to use shadcn-vue directly in bloc app. This will show if it's an issue related to shadcn-vue being used in a monorepo lib that is imported by an app like shown above.

@olemarius
Copy link
Contributor Author

@zernonia I've set up a reproducable repo here https://github.com/olemarius/shadcn-vue/tree/injectionkey-error-demo

Just pull, run pnpm i and try to run the build command in the project repo and you should get this error:

image

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Nov 19, 2023

@olemarius You should use vue-tsc instead of tsc

Here is your GitHub project as StackBlitz Playground

https://stackblitz.com/edit/github-jthw4x


If you have this issue unovue/radix-vue#229 @zernonia could consider exporting createContext composable from radix-vue


BTW I tested the Form component in Dev and Prod mode in Nuxt projects it didn't have that warning ("Symbol" not found.)

https://stackblitz.com/edit/github-xkryhq-egbi1f

@olemarius
Copy link
Contributor Author

@sadeghbarati thanks, I got it to build but still get the Symbol not found error.

@olemarius
Copy link
Contributor Author

@sadeghbarati I created a PR that adds vue-tsc script to apps/www/package.json, ensuring that there's no vue-tsc validation errors when developers pull down shadcn-vue coponents.

I fixed the errors that came up both in demos and in Calendar component.

@olemarius olemarius mentioned this issue Nov 19, 2023
@olemarius
Copy link
Contributor Author

@sadeghbarati One noteworthy thing about changing to vue-tsc is that it doesnt support js emits, and tsc throws the import from *.vue error, so the only way I found was to add ts-ignore manually, and do tsc && vue-tsc --noEmit && vite build. If you know a way around this that doesnt involve manually adding ts-ignore, please let me know 🙏🏼

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Nov 22, 2023

(@bloc/ui(radix-vue(shadcn-vue))) 😶‍🌫️

in Vue vue-tsc is enough for type check .vue and even .ts files

I think you were using Monorepo right? So you are trying to build a package Wrapper (radix-vue) of another Wrapper (shadcn-vue)? but by doing this you are breaking the main duty of shadcn (Copy Component Library)


Short answer: don't

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Nov 22, 2023

(medusa-ui(radix-ui)) this looks fine If you want to only wrap around one package (@bloc/ui(radix-vue))

Take a look at medusa design-system
https://github.com/medusajs/medusa/blob/develop/packages/design-system/ui/package.json

@olemarius
Copy link
Contributor Author

@sadeghbarati yep it's a monorepo, and the main app 'bloc' has become a giant app that I'm planning to split up into multiple, which is why I was thinking that creating a wrapper for shadcn-vue would make sense. I definetively see your point, that it'd be better run the cli / copy for each app in the monorepo using shadcn-vue.

But let's say I end up with 5 apps and we do small adjustments on many of the components, then run the cli update command. This will overwrite all the local changes, requiring us to go trough and make sure none of the adjustments are lost before committing. If updating the components via cli worked similar to git auto merge and would guide you trough any conflicts, it'd make updating a lot smoother.

But regardless, I'll think about moving shadcn-vue into each app. Thanks for the feedback and examples btw! 🙏🏼

@olemarius
Copy link
Contributor Author

Actually it looks like the original shadcn-ui has some experiemntal diffing feature going on, so I'm guessing this will be adopted by shadcn-vue once stable, ref https://ui.shadcn.com/docs/cli

I'll close this issue as I agree that my monorepo setup is not by shadcn-vue's design.

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