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

experiment: Use typebox for build-tools config schema validation #23566

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tylerbutler
Copy link
Member

@tylerbutler tylerbutler commented Jan 15, 2025

Summary

This change converts the types used for config in the build-tools package to use typebox for type generation and validation. With this in place, configs that do not conform to the schema will be rejected.

I made this change to learn more about typebox in a familiar environment. If none of this is merged, I won't be sad - this is exploratory.

Reviewer Guidance

Please ignore completeness of this PR and focus on the core changes to the config types and how I used typebox. In particular, I would like input/feedback on the following:

  1. The types that typebox generates, while correct type-wise, are much more verbose in intellisense and api-extractor reports because of the type wrapping. How can/should we improve the UX of using the types? (maybe generate them differently?)
  2. Order of the type declarations is now more relevant - can't use types unless they're defined earlier in the file. I assume this is because of the different mechanics around type creation now.
  3. There are cases where the config uses types like RegExp, and that works because our configs are JS themselves. (probably just an example of where this use-case might not fit typebox exactly - but this is just for learning).
  4. Docs - not sure how doc comments work with typebox types.
  5. This exploration uses the typebox static API; can this be hidden from API users? Ideally customers shouldn't need to use a special syntax with their types to get validation - but maybe we can abstract this all away in wrapper types?

@github-actions github-actions bot added base: main PRs targeted against main branch area: build Build related issues public api change Changes to a public API labels Jan 15, 2025
repoPackages?: IFluidBuildDirs;
version: typeof BUILDPROJECT_CONFIG_VERSION;
}
export type BuildProjectLayout = Static<typeof BuildProjectLayout>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend not exporting the actual types used by your persisted data formats.

There are two patterns I'm aware of for this that work in different cases:

  1. When the data is only used in one relatively localized place and the only interaction with that places is via serialized data, simply don't export the interface or the schema. Instead have that code take in data in the persisted format. It then uses the schema to parse the data and report any errors and uses the parsed data as it needs internally: nothing related to the schema is exported, with the possible exception of documentation if humans need to hand edit data in this format, and possibly a json schema generated from the schema object (for human and/or machine consumption).
  2. If you need to have an in memory representation that has clean types, declare that as a normal type independent from the schema for parsing data. Then follow the above pattern, but copy the data into the in memory format to expose it in various APIs. This allows the in memory format to be evolved (have refactor renames etc) without breaking compat with persisted data, and also allows the in memory format to use Maps, sets and other non JSON compatible features. The persisted format can reference the more public in memory format to link documentation.

Regardless of which approached is used, its generally a good idea to keep the schema (and any dependencies like string constants they use) in specially marked files with minimal dependencies so its easy to tell when a change might impact compat with persisted data. Another approach to this is snapshot testing of json scheme exported from the schema so changes to what they accept will break tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: build Build related issues base: main PRs targeted against main branch public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants