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(jsonify): fix array jsonify handler #673

Merged
merged 4 commits into from
Sep 30, 2023

Conversation

Emiyaaaaa
Copy link
Collaborator

@Emiyaaaaa Emiyaaaaa commented Aug 26, 2023

hey!this is my first contribution

  1. fix Jsonify: readonly messes with Jsonified tuple types #629
  2. fix another jsonfity array bug
// main branch
declare const mixTupleJson: Jsonify<['1', typeof fn, 2]>; // type: ['1', 2]
// this pr
declare const mixTupleJson: Jsonify<['1', typeof fn, 2]>; // type: ['1', null, 2]

// because `FilterNonNever` filter all NotJsonable value. but in array, NotJsonable value should be `null`

@sindresorhus
Copy link
Owner

fix another jsonfity array bug

Would you be able to split this out into a separate pull request? That would make it easier to review.

@sindresorhus
Copy link
Owner

// @MichaelDeBoey @sachinraja

@Emiyaaaaa
Copy link
Collaborator Author

Emiyaaaaa commented Aug 26, 2023

fix another jsonfity array bug

Would you be able to split this out into a separate pull request? That would make it easier to review.

  1. fix Jsonify: readonly messes with Jsonified tuple types #629 bug just make array and readonly array use same handler.
  2. but previous array handler JsonifyList is wrong, this handler filter all NotJsonable value, that will make JsonifyList<Array<number>> type to never[] but not null[], it's obviously not right.
  3. but, how previous jsonify array test passed ?
    https://github.com/sindresorhus/type-fest/blob/main/source/jsonify.d.ts#L119C1-L119C1 this line is wrong, not all array match this rule T extends [unknown, ...unknown[]] like Jsonify<Array<number>>, thay match next rule T extends ReadonlyArray<infer U>, this handler just right for Array<...>, wrong for readonly [1,2,3]

so i fixed JsonifyList type, and i think this is another bug. maybe my thoughts is wrong and i fell split this is a little difficult

sorry about that

@MichaelDeBoey
Copy link
Contributor

CC/ @pcattori

test-d/jsonify.ts Outdated Show resolved Hide resolved
test-d/jsonify.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Can you fix the merge conflict?

@Emiyaaaaa
Copy link
Collaborator Author

Can you fix the merge conflict?

fixed

@sindresorhus sindresorhus merged commit 025f6e9 into sindresorhus:main Sep 30, 2023
6 checks passed
@sindresorhus
Copy link
Owner

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.

Jsonify: readonly messes with Jsonified tuple types
3 participants