fabian-hiller / valibot

The modular and type safe schema library for validating structural data 🤖
https://valibot.dev
MIT License
6.33k stars 204 forks source link

add `rawCheckAsync` & `RawCheckActionAsync` API refs #831

Closed EltonLobo07 closed 2 months ago

EltonLobo07 commented 2 months ago

This PR adds rawCheckAsync and rawCheckActionAsync API references. While adding content, something was unclear to me. The question I asked myself was: Should I create a 'type' page (for example, IntersectOptions) for the Context type used by the only parameter of the function passed to this action, or should I inline the type? The rawCheck API reference inlined the type definition, and so I did the same. I assume the reason for inlining the type definition for the rawCheck API reference was to avoid polluting the 'types' section on the website with local types that are associated with maybe just one action so that we reduce the chances of naming collisions? For example, there is a Context type in the _addIssue.ts file. If someday we were to create a 'type' page for it along with the Context type associated with the rawCheck action, it wouldn't be directly possible due to conflicting names. But inlining type definitions might also not be a good idea. If the Context type associated to the rawCheck and rawCheckAsync actions changed, we will have to update the content in both places. Can you clarify when I should create a dedicated 'type' page for a type definition?

pkg-pr-new[bot] commented 2 months ago

Open in Stackblitz

pnpm add https://pkg.pr.new/valibot@831

commit: 990cfb3

fabian-hiller commented 2 months ago

To be honest, I didn't think much about it when I added rawCheck to the API reference. I inlined it because it is a private type that conflicts with other types. I agree that the inline type is not really readable, but adding a separate page to the (types) folder would force use to append a suffix to prevent name conflicts.

We could add such private types that are not used globally directly to the path of the API. So instead of adding a new route to /api/(types), we add a new one to /(actions)/rawCheck. This results in /api/rawCheck/Context to access the page. What do you think?

EltonLobo07 commented 2 months ago

I think that's a great idea! It will definitely increase the readability and maintainability of the website. There's just one concern: Which section would you list the type in (on the sidebar)? The most suitable is the Types list. image But like you said, there may be name conflicts here too (now or in the future). How would you work around this?

fabian-hiller commented 2 months ago

I would not list these types in the menu at all, as they are internal types and not accessible from the outside. People should only find them by clicking directly on them in the API reference. What do you think?

EltonLobo07 commented 2 months ago

I agee. This way we won't end up creating a very large list where some of the types are very rarely used (viewed), as they are mostly internal details. Let me know if I can make the discussed changes to this PR.

fabian-hiller commented 2 months ago

Yes, go ahead! Thank you Elton!

EltonLobo07 commented 2 months ago

 The next, previous and edit page links are missing for the rawCheck internal type pages. Since the internal type pages won't be listed, I think we don't need the next and previous page links. We definitely need the edit page link but I am not sure how to add it. Can you take the task of creating a layout comp and setting it up for internal type pages as I am not familiar with Qwik or I can do it after reading the Qwik documentation which might take some time. If you want me to do it, can you provide a summary of how you would have done it to make sure we are on the same page?

fabian-hiller commented 2 months ago

I will take a look at it

fabian-hiller commented 2 months ago

The layout is automatically derived from the parent. The reason the edit button does not appear is different. It would ignore it for now since only a few pages are affected.