-
-
Notifications
You must be signed in to change notification settings - Fork 239
feat: Shadcn Registry Package #1188
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
base: next
Are you sure you want to change the base?
Conversation
|
@I-3B is attempting to deploy a commit to the 47ng Team on Vercel. A member of the Team first needs to authorize it. |
|
Thanks! I was working on the docs part (listing of installable items and copy/paste vs CLI methods) while on the plane, I'll open a PR later today. |
|
hi @franky47 ! |
|
Yeah I'll open a PR to the shadcn registry, as we have the green light from the man himself, but once #1189 is live. I have opened #1189 as a draft too for the docs UI part, we could stack yours on top of that one to add parseAsTuple. At a first glance, I'm not super keen on widening the exported API for internal purposes, I'll have to take a closer look once I'm home. But in #1189 I have all the "sources" for the registry as part of where it is deployed (in the docs package), and that causes problems in terms of dependencies, so a separate package might make more sense. Like I wanted to add the equivalent of the next-typed-links snippet for React Router's href utility, but that would have implied adding the react-router dependency to the docs, along with tests harnesses etc.. Better do a separate package, have it build the registry JSON files and mark it as a dependency of the docs. We'll still need to copy the built output into the docs' |
agree, alternative is to add a |
…mplement parseAsTuple tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't add debug file as I thought it's an internal package detail that shouldn't be put in users' files, tradeoff of course is that debug flag doesn't work with community parsers
franky47
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I merged #1189, feel free to rebase on top of that.
It might make sense to expose the safeParse function as initially suggested (which would also make the feat: prefix a legitimate minor bump for the nuqs package).
packages/registry/registry.json
Outdated
| "path": "parsers/parseAsTuple.ts", | ||
| "type": "registry:file", | ||
| "target": "~/components/parsers/parseAsTuple.ts" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: This would also need to install the safeParse utility as it's used in the parseAsTuple file.
Maybe it would make sense to export it from nuqs then, as it's useful for custom parsers anyway.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, there has been a few changes in how the registry is built in #1209:
- Each item is now described in an individual
packages/docs/src/registry/{item-name}.json - Next to that file can be an optional
{item-name}.mdfor usage docs - Those items can specify a URL for file paths, in which case its contents will be fetched when building the registry (we use this for the Inertia adapter which lives in a PHP example monorepo).
I think we could modify the "assemble" script that pulls all these items together into a generated root registry.json file to also go read from packages/docs/node_modules/registry/items/**/*.json (symlinked to packages/registry/items/**/*.json) or something similar, to enforce good boundaries between packages: registry for testing items, docs for deployment.
I thing there would still be cases where items could live:
- In the docs directly, if simple enough, or if an integration with Next.js is needed (eg: typed links)
- Outside the repo, if the test harness pulls too many dependencies that would slow down CI (eg: adapters for One.js or Expo Router, we might not want to add a full React Native stack to this monorepo just for one copy-pastable block of code).
But there is a lot of value in having items living in a testable registry package (or set of packages if we see in the future that some test harnesses conflict with one another).
I'm going to be preparing for a talk at React Advanced at the end of the week, so I might not be super active in PRs, but if you need guidance in how to adapt this PR to fit that model, feel free to ping me here. I've left some comments here and there too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: This one would be difficult to test without having a proper Next.js setup to typegen route definitions from, which we do have in the docs and could test against there.
|
Thanks for the review and good luck with the talk!
even if the item is simple enough, we still would want to have it tested, yes? (e.g. parseAsTuple), so either we add tests right in docs package, or move all items, even simple ones, to registry package as they need to be tested. My worry is that it will be hard to maintain 2 resources of registry items with no clear distinction on why a particular item is under docs or registry. so need to decide if we want to discard the registry package altogether, and keep everything under docs (with their tests), or move all items to be under registry package, and update assemble script to pull from registry package. or something else? |
|
I was thinking of the nextjs-typed-routes, which pulls type definitions from the |
|
I see, okay then as a start I'll update the registry to work with the assemble script, might also move the current registry items that are under docs to be under registry (except nextjs-typed-routes). |
2dfd858 to
081f9ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the registry dependency looks much better. However, it looks like the usage markdown file wasn't picked up:
Edit: yeah the readUsage function still expects it to be located in the docs registry directory:
https://github.com/47ng/nuqs/blob/next/packages/docs/src/registry/read.ts#L35-L36
| parseAsTuple([parseAsInteger, parseAsInteger]) | ||
|
|
||
| // Optionally, customise the separator | ||
| parseAsTuple([parseAsInteger, parseAsInteger], ';') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: we could showcase a combination of different parsers in those docs, like [string, 'asc' | 'desc'] for a sorting tuple or something similar.
closes #1022 #1036 #1118
Checklist
parseAsTupleregistry itemparseAsTupleparseAsTuple