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(remix-server-runtime): use type-fest's Jsonify type in Serialize #6642

Conversation

MichaelDeBoey
Copy link
Member

@MichaelDeBoey MichaelDeBoey commented Jun 20, 2023

  • Extracted Jsonify so that it's easier to switch to type-fest at a later stage
  • Added cases for positive/negative infinity, Boolean, Number, String, Map & Set
  • Added more deep grained type for objects that have a toJSON function
  • Update UndefinedToOptional type
  • Renamed some generics so they're more in line with the type-fest codebase
    • NonJsonPrimitive -> NotJsonable
      • Also replaced Function with (...arguments_: any[]) => any
    • SerializeTuple -> JsonifyList
      • Also updated this
    • SerializeObject -> JsonifyObject
      • Also updated this

This is mainly taking over @sachinraja's awesome work from sindresorhus/type-fest#519, thanks for that! 🙏

@changeset-bot
Copy link

changeset-bot bot commented Jun 20, 2023

⚠️ No Changeset found

Latest commit: aa5a23e

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

@MichaelDeBoey MichaelDeBoey force-pushed the bring-Serialize-type-in-line-with-type-fests-Jsonify branch 2 times, most recently from 7ed5847 to cd65f45 Compare June 20, 2023 00:59
@MichaelDeBoey MichaelDeBoey force-pushed the bring-Serialize-type-in-line-with-type-fests-Jsonify branch from cd65f45 to f3e230e Compare June 20, 2023 01:18
@brophdawg11 brophdawg11 removed their request for review June 20, 2023 20:31
@MichaelDeBoey MichaelDeBoey force-pushed the bring-Serialize-type-in-line-with-type-fests-Jsonify branch from f3e230e to 718e59c Compare June 26, 2023 23:37
@pcattori
Copy link
Contributor

pcattori commented Jul 3, 2023

@MichaelDeBoey given that we have abandoned pretty types (at least for now), I think we are set to adopt type-fest types for JSON serialization, so happy to review if you updated this PR!

@MichaelDeBoey MichaelDeBoey force-pushed the bring-Serialize-type-in-line-with-type-fests-Jsonify branch from 718e59c to 41e4c3b Compare July 3, 2023 22:46
@MichaelDeBoey
Copy link
Member Author

@pcattori I also applied all the changes from this repo into type-fest, but that does seem to break a test case ( see sindresorhus/type-fest#638)
Any idea what would be the fix?

I'd love to get it fixed upstream & here

@pcattori
Copy link
Contributor

pcattori commented Jul 7, 2023

@MichaelDeBoey left a fix for the upstream type-fest PR

@MichaelDeBoey MichaelDeBoey force-pushed the bring-Serialize-type-in-line-with-type-fests-Jsonify branch from bc5d5f2 to 80713d6 Compare July 7, 2023 15:17
@MichaelDeBoey
Copy link
Member Author

@pcattori I've updated this PR to the latest version of what I've done in sindresorhus/type-fest#638 as well

@pcattori
Copy link
Contributor

pcattori commented Jul 8, 2023

@MichaelDeBoey I think the goal for this PR should be to replace our types with type-fest entirely after the upstream PR lands, rather than just update our types to match theirs.

@MichaelDeBoey
Copy link
Member Author

MichaelDeBoey commented Jul 9, 2023

@pcattori Unfortunately that's not possible (yet) as type-fest requires Node@>=14.16, which would be a breaking change as all our packages currently require Node@>=14.0.0
In v2, we can use type-fest as our minimum Node requirement would be 18

type-fest also requires Typescript@>=4.7
As we're currently not explicit about which versions we support, I don't think this is a problem though

@MichaelDeBoey
Copy link
Member Author

@pcattori It seems like there's still a problem with the Jsonify type 🤔
sindresorhus/type-fest#638 (comment)

@MichaelDeBoey MichaelDeBoey force-pushed the bring-Serialize-type-in-line-with-type-fests-Jsonify branch 4 times, most recently from 0867207 to 0c38d30 Compare August 1, 2023 18:50
@MichaelDeBoey MichaelDeBoey force-pushed the bring-Serialize-type-in-line-with-type-fests-Jsonify branch 2 times, most recently from 00dae7a to 6f44b56 Compare August 3, 2023 00:46
@pcattori
Copy link
Contributor

pcattori commented Aug 3, 2023

@MichaelDeBoey since dev branch is now for v2 development and requires Node >=18 and TS >= 5.1, can we now switch this to just pull in the types from type-fest?

@MichaelDeBoey MichaelDeBoey force-pushed the bring-Serialize-type-in-line-with-type-fests-Jsonify branch from 6f44b56 to 1b63cb6 Compare August 3, 2023 12:10
@MichaelDeBoey MichaelDeBoey changed the title feat(remix-server-runtime): bring Serialize type more in line with type-fest's Jsonify type feat(remix-server-runtime): use type-fest's Jsonify type in Serialize Aug 3, 2023
@MichaelDeBoey
Copy link
Member Author

@pcattori I now changed this PR to use type-fest

@MichaelDeBoey MichaelDeBoey added the v2 Issues related to v2 apis label Aug 3, 2023
@MichaelDeBoey MichaelDeBoey force-pushed the bring-Serialize-type-in-line-with-type-fests-Jsonify branch 4 times, most recently from de3a1d5 to 1d61ff8 Compare August 5, 2023 15:04
@MichaelDeBoey MichaelDeBoey force-pushed the bring-Serialize-type-in-line-with-type-fests-Jsonify branch from 1d61ff8 to 23f38a1 Compare August 7, 2023 19:49
@pcattori pcattori merged commit 40a687e into remix-run:dev Aug 8, 2023
9 checks passed
@MichaelDeBoey MichaelDeBoey deleted the bring-Serialize-type-in-line-with-type-fests-Jsonify branch August 8, 2023 17:01
@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-a179aa7-20230809 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-b1149bb-20230810 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@smeijer
Copy link
Contributor

smeijer commented Oct 2, 2023

Would it make sense to revert this PR till there is a stable solution? The Jsonify thing seems to be a serious issue. A lot of loaders now return Jsonify({}), while they were fully typed in Remix v1.9.x.

FYI, I'm typing my loaders using the DataFunctionArgs type, like:

export async function loader({  }: DataFunctionArgs) {
  return defer({ obj });
}

Just applied a patch-package, and all my trouble are gone

https://gist.github.com/smeijer/885310fca21828272bca29097f490534

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

3 participants