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

feat: bring back simplified types for SerializeFrom #6746

Closed
wants to merge 1 commit into from

Conversation

afoures
Copy link

@afoures afoures commented Jul 1, 2023

just saw that #6736 removed pretty types for SerializeFrom because of circular dependencies and infinitely recursive types.

this is an attempt to fix the infinitely recursive issue without loosing type simplification.

i tested it using the same Typescript playground.

there is one issue for circular types, where we loose the type name: in the playground, the name of Stripe.InvoiceLineItem is lost.

this happen when [T] extends [U] in type PrettyTransform<T, U> = [T] extends [U] ? T : Pretty<U>; is evaluated to false, and that is the case when T is a infinite circular type, at one point U will have the circular property evaluated to never because max depth is reached, so T and U are not the same anymore. i'm not sure how we could fix it.

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jul 1, 2023

Hi @afoures,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@changeset-bot
Copy link

changeset-bot bot commented Jul 1, 2023

⚠️ No Changeset found

Latest commit: 29045ba

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@afoures afoures changed the base branch from main to dev July 1, 2023 07:56
@afoures afoures force-pushed the add-pretty-types branch from 8157d6d to 7fc7963 Compare July 1, 2023 08:00
@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jul 1, 2023

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@pcattori
Copy link
Contributor

pcattori commented Jul 3, 2023

Thank you for looking into this @afoures 🙏 Cool to see an example of a TS counter (via Depth array type) in the wild.

Summary of type expectation mismatches with this approach:


Screenshot 2023-07-03 at 9 20 40 AM

👆 like you already mentioned, inferred types for circularly referenced like Stripe.InvoiceListItem do not preserve the original type names


Screenshot 2023-07-03 at 9 19 57 AM

👆another example of the same thing, this time with Json circularly referenced type


Screenshot 2023-07-03 at 9 20 10 AM

👆 Haven't checked if this is another example yet or its own issue, but type names for Zod are not wholly preserved either


Currently, I'm not convinced that the added complexity here is worth it when it doesn't solve the issue for any circularly referenced types. While it does improve the DX slightly for cases where there are no circularly referenced types, it creates even more confusion once users do include such a type. And they may not even be aware that the type is circular!

Circularly referenced types are not rare (e.g. Zod and Stripe are quite popular), so I think the slight DX improvement in some cases is outweighed by the noticeable confusion that would cause DX degradation.

@afoures
Copy link
Author

afoures commented Jul 3, 2023

You're right, I did not see that Json was broken too. Should I try to fix circular type issues?

@pcattori
Copy link
Contributor

pcattori commented Jul 3, 2023

@afoures you are more than welcome to and I'm definitely curious what a solution would look like! That said, if the solution turns out to be significantly complex, we'll have to weigh the trade-offs carefully.

@pcattori
Copy link
Contributor

pcattori commented Jul 3, 2023

@afoures closing this for now, but if you come up with a solution for circular types, I'm happy to reopen.

@pcattori pcattori closed this Jul 3, 2023
@pcattori
Copy link
Contributor

pcattori commented Jul 3, 2023

One more thing is that we are trying to move to type-fest for JSON serialize/deserialize types rather than maintaining our own. Adding pretty types in the middle of the type recursion means we won't be able to do that anymore. So maybe a better place to land any pretty types would be there, though the same/similar trade-offs apply whether its here or there.

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

Successfully merging this pull request may close these issues.

2 participants