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-run/cloudflare,@remix-run/deno,@remix-run/node): SerializeFrom utility for loader and action type inference #4013

Merged
merged 19 commits into from
Aug 18, 2022

Conversation

pcattori
Copy link
Contributor

@pcattori pcattori commented Aug 17, 2022

Renamed internal UseDataFunctionReturn type to SerializeFrom since its can be used without useLoaderData and useActionData. A bunch of small type cleanups + bug fixes. Moved serialize type utilities to @remix-run/server-runtime and re-exporting SerializeFrom and TypedResponse from each runtime pkg.


  • Docs
  • Tests
  • Changeset

Testing Strategy:

  • updated existing hooks types tests in @remix-run/react
  • locally created an app with indie stack, installed local @remix-run/server-runtime and @remix-run/node, imported SerializedFrom from @remix-run/node, and got the correct type in VS Code

@changeset-bot
Copy link

changeset-bot bot commented Aug 17, 2022

🦋 Changeset detected

Latest commit: 65aceb4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
remix Major
@remix-run/cloudflare Major
@remix-run/deno Major
@remix-run/node Major
@remix-run/react Major
@remix-run/serve Major
@remix-run/server-runtime Major
@remix-run/cloudflare-pages Major
@remix-run/cloudflare-workers Major
@remix-run/architect Patch
@remix-run/express Major
@remix-run/netlify Major
@remix-run/vercel Major
@remix-run/dev Major
create-remix Major
@remix-run/eslint-config Major

Not sure what this means? Click here to learn what changesets are.

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

@pcattori pcattori force-pushed the pedro/serialized-from branch 3 times, most recently from bed12e9 to c512e1d Compare August 17, 2022 20:04
@pcattori pcattori requested a review from chaance August 17, 2022 20:05
@pcattori pcattori force-pushed the pedro/serialized-from branch from c512e1d to 377512a Compare August 17, 2022 20:08
@pcattori pcattori marked this pull request as ready for review August 17, 2022 20:09

function isEqual<A, B>(
arg: A extends B ? (B extends A ? true : false) : false
): void {}

// not sure why `eslint` thinks the `T` generic is not used...
// eslint-disable-next-line @typescript-eslint/no-unused-vars
type LoaderData<T> = ReturnType<typeof useLoaderData<T>>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: this test is for testing useLoaderData's types, so we actually grab types from useLoaderData instead of assuming its using UseDataFunctionReturn or SerializeFrom.

@pcattori pcattori force-pushed the pedro/serialized-from branch 2 times, most recently from 7e2556c to fc97ee4 Compare August 18, 2022 19:34
@pcattori pcattori changed the title feat(@remix-run/react): SerializeFrom utility for loader and action type inference feat(@remix-run/cloudflare,@remix-run/deno,@remix-run/node): SerializeFrom utility for loader and action type inference Aug 18, 2022
@@ -32,7 +32,6 @@ export type {
CookieParseOptions,
CookieSerializeOptions,
CookieSignatureOptions,
CreateRequestHandlerFunction,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CreateRequestHandlerFunction is a utility for authoring Remix runtime packages and was incorrectly re-exported before.

@@ -28,7 +28,6 @@ export type {
IsSessionFunction,
JsonFunction,
RedirectFunction,
TypedResponse,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The interface section of exports is for types related to values/functions that should be exported by every server runtime pkg. We use the fetch API for request/response, so we don't export our own request/response implementations from server runtime pkgs.

Therefore, TypedResponse should not be considered part of the server runtime interface.


TBH the distinction between "interface" and "re-export" is subtle and I'd much prefer to not need re-exports at all once we have a pkg for exporting runtime-agnostic stuff.

Copy link
Collaborator

@chaance chaance left a comment

Choose a reason for hiding this comment

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

Just one note on the changeset, otherwise looks good!

.changeset/neat-beds-unite.md Outdated Show resolved Hide resolved
@pcattori pcattori force-pushed the pedro/serialized-from branch from acecb4c to bb68c13 Compare August 18, 2022 21:32
convention is to use plurals for collections (e.g. arrays), not for unions
arrays are technically `object`s, so we need something more specific. `Record<PropertyKey, unknown>`
should match any object, but not match arrays.
…ToOptional`

more obvious that this _converts_ `undefined` unions to optionals
…utility

and to be consistent with `SerializeObject`
and provide comments to explain what tuples are and how `object` matches classes
makes each line in the type definition independent of other lines.
more readable and easier to comment/uncomment specific conditions.
plus less git noise when adding/removing/reordering conditions.
@pcattori pcattori force-pushed the pedro/serialized-from branch from bb68c13 to 65aceb4 Compare August 18, 2022 21:32
@pcattori pcattori requested a review from chaance August 18, 2022 22:06
@pcattori pcattori merged commit 5c9539d into dev Aug 18, 2022
@pcattori pcattori deleted the pedro/serialized-from branch August 18, 2022 22:07
@MichaelDeBoey MichaelDeBoey removed the awaiting release This issue has been fixed and will be released soon label Aug 25, 2022
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.

3 participants