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

Fix coercion types #683

Merged
merged 1 commit into from
Feb 20, 2024
Merged

Fix coercion types #683

merged 1 commit into from
Feb 20, 2024

Conversation

tmcw
Copy link
Contributor

@tmcw tmcw commented Feb 16, 2024

This fixes the ability for fast-json-stringify to coerce Dates into strings and BigInts into numbers. These type tests weren't having any effect before: when schemas are declared like

// Number schemas
const schema1: Schema = {
    type: 'number'
}
const schema2: Schema = {
    type: 'integer'
}

build(schema1)(25)
build(schema2)(-5)

schema1 and schema2 are typed simply Schema, so none of the nice type inference is triggered and you can pass any argument to the method, like

build(schema1)({ xyz: 1 })

And there will be no type error. This adds some duplication because the two options that I know of are either adding as const to the end of each schema declaraction, or using a literal. as const doesn't work in this situation because it makes the type readonly, which then isn't accepted by build().

This disables a bunch of tests here: https://github.com/fastify/fast-json-stringify/pull/683/files#diff-72016aeffe80c3b259bbbf7e92a664cd2bcf49c2b79597740b4dcd8e430107bdR161 which are breaking for a different reason - the type declaration itself has always, afaict, been wrong, and there isn't an easy way to fix it. This PR just uncovered that pre-existing issue.

Checklist

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@tmcw
Copy link
Contributor Author

tmcw commented Feb 20, 2024

Thanks! Is there additional process for getting this merged?

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

lgtm

I still refrain from merging type PRs 😄, so @Uzlopak will do it maybe?

@mcollina mcollina merged commit 5d49f80 into fastify:master Feb 20, 2024
19 checks passed
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.

Date coercion isn't supported by TypeScript types
3 participants