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

Tuple describe() method #1947

Merged
merged 6 commits into from
Mar 23, 2023
Merged

Conversation

DaddyWarbucks
Copy link
Contributor

This PR addresses: #1891

The code aims to standardize how Array and Tuple inner types are configured. Historically array has stored it's one inner type on a prop innerType. Tuple has stored its types in spec.types. The types MUST be stored on the spec object for Tuple because it is needed for the notType method. See:

notType: (params) => {

Both Array and Tuple now have schema.innerType and schema.spec.innerType. This was done for backwards compatibility and consistency. And all corresponding methods and types have been updated.

This now allows the Tuple to return information about its types in the describe method

const description = tuple([string(), number()]).describe();

description = {
  type: 'tuple',
  innerType: [
    { type: 'string },
    { type: 'number' }
  ]
}

Let me know if you have any questions!

src/locale.ts Outdated
@@ -136,7 +136,7 @@ export let array: Required<ArrayLocale> = {
export let tuple: Required<TupleLocale> = {
notType: (params) => {
const { path, value, spec } = params;
const typeLen = spec.types.length;
const typeLen = spec.innerType.length;
Copy link
Owner

Choose a reason for hiding this comment

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

i'd rather keep the types name, i wanted to migrate array to use it it as well but opted to avoid the breaking change

Copy link
Contributor Author

@DaddyWarbucks DaddyWarbucks Mar 17, 2023

Choose a reason for hiding this comment

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

OK. I can change that back to types. I suppose that is actually a breaking change too, so it should remain types. Its a bummer I didn't get to this before the official release of V1 🤷

What should I do with Array?
1 - Return it to its current shape (no spec)
2 - Add spec.innerType = {...the one type}
3 - Add spec.types = [{ ...the one type }] // note the array
4 - Add spec.types = { ...the one type } // note no array
5 - Add spec.type = { ...the one type } // note spec is singular

I am leaning towards option 3. It seems most consistent with Tuple. And if there is ever another use case for some "inner type", then the property types with an array is flexible.

Copy link
Owner

Choose a reason for hiding this comment

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

I think 3 makes sense to me 👍

@DaddyWarbucks
Copy link
Contributor Author

While I've got your eyes on this, this appears to be a bug in the array schema:

base.innerType = this.innerType.describe(options);

Shouldn't that pass innerOptions?

@DaddyWarbucks
Copy link
Contributor Author

@jquense All updates have been made. let me know if you have anymore questions or concerns.

@jquense jquense merged commit 297f168 into jquense:master Mar 23, 2023
@jquense
Copy link
Owner

jquense commented Mar 23, 2023

thanks!

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

Successfully merging this pull request may close these issues.

2 participants