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!: align types with React Router #7319

Merged
merged 10 commits into from
Sep 6, 2023

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Aug 31, 2023

Aligning types with those used in React Router to remove the confusion around LoaderArgs/LoaderFunctionArgs etc.

See the changeset for the details and this comment for the desired state

@changeset-bot
Copy link

changeset-bot bot commented Aug 31, 2023

🦋 Changeset detected

Latest commit: 41a1647

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

This PR includes changesets to release 16 packages
Name Type
@remix-run/cloudflare Major
@remix-run/deno Major
@remix-run/node Major
@remix-run/react Major
@remix-run/server-runtime Major
@remix-run/cloudflare-pages Major
@remix-run/cloudflare-workers Major
@remix-run/architect Major
@remix-run/express Major
@remix-run/serve Major
@remix-run/testing Major
@remix-run/dev Major
create-remix Major
remix Major
@remix-run/css-bundle 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

@brophdawg11 brophdawg11 force-pushed the brophdawg11/align-rr-types branch from 389c5ff to 77a8b65 Compare August 31, 2023 16:32
@MichaelDeBoey
Copy link
Member

  • Typed as any

@brophdawg11 I would argue that we should update RR to use unknown instead
https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html#any

Since that would be a breaking change on the RR side, I think it would be best to rename ActionArgs to ActionFunctionArgs and LoaderArgs to LoaderFunctionArgs and implement it here like

export type AppLoadContext = unknown;
export type DataFunctionArgs<Context = unknown> = {
  context: Context;
  params: Params;
  request: Request;
}

Once we also updated this in RR (this should be done in a major version), we can remove it here and just re-export RR types as you wanted to do in this PR

@brophdawg11
Copy link
Contributor Author

I'd prefer not to replace one set of duplicated types with another. If any is truly not what we want, I'd rather just keep it as is in Remix today I think.

I think naming the exports the same if they aren't identical is more confusing since then LoaderFunctionArgs is different depending on where you import it form. With VSCode auto-complete very often incorrectly recommending react-router-dom higher than @remix-run/react in the list - folks may end up with the wrong one unknowingly. I think the current state of LoaderArgs/LoaderFunctionArgs is actually a benefit if they're different.

FWIW we have chosen any specifically before. Using unknown is certainly stricter and "better" from a TS purist point of view, but it also feels sometimes like when we type something as unknown, we force all consumers to be purists as well. I'll defer to @mjackson and @ryanflorence on any official preference though.

@brophdawg11 brophdawg11 added the v2 Issues related to v2 apis label Aug 31, 2023
Copy link
Member

@mjackson mjackson left a comment

Choose a reason for hiding this comment

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

LGTM. Only thing I think we could probably do a bit better is use unknown for data we get from the user in AppData, but it isn't a huge deal and would probably break some people's typechecking, so I'd say let's just leave it as any for now.

@MichaelDeBoey
Copy link
Member

MichaelDeBoey commented Aug 31, 2023

I'd rather just keep it as is in Remix today I think

The thing is: if we want to make it unknown at some point, that would be a breaking change, as unknown requires an explicit type check, any doesn't require anything at all

I get the idea of any being "easier to use", but imo that would people give a wrong feeling of (type) safety, so I think we should go with unknown, even if that means people will be forced to do extra checks
Isn't the whole thing about Remix "we learn you how to do it the proper way (together with using web standards)"?
Having the extra checks is "the proper way", so the way to go imo

@brophdawg11
Copy link
Contributor Author

brophdawg11 commented Sep 1, 2023

I don't have a strong preference either way really, I just would like us to be consistent on types we don't know - stuff the user hands to us somewhere and we provide back to them elsewhere - context, loaderData, handle, location.state (I'm sure there are other's I'm not thinking of at the moment).

The current state is:

React Router Remix
Context any { [key: string]: unknown }
useLoaderData unknown any (via generic)
handle any any
location.state any any (inherited)

So do we:

  1. Align on any today since it's the smallest change and also does not require a breaking RR change?
    • We can add a generic to DataFunctionArgs so folks can type it stronger than any
    • Then in the near future we can export new types based on unknown, deprecate the any ones, and drop the any ones in the next major version of both RR and Remix
  2. Or, move Remix to unknown in v2, and follow the deprecation path to move RR to unknown in v7
    • This means we'll be exporting duplicate types (LoaderFunctionArgs will be different in Remix than it is in React Router - but maybe only in the default value for the generic)

Also worth keeping in mind is the much better typings coming in some of our v3 APIs, if that plays a role in how strong we try to make the v2 types.

@mjackson
Copy link
Member

mjackson commented Sep 1, 2023

I'd personally go with option 2 since I think that's where we eventually want to be.

@brophdawg11 brophdawg11 self-assigned this Sep 1, 2023
@MichaelDeBoey
Copy link
Member

I agree option 2 (using unknown in Remix v2, same in RR v7 and upgrading to RR types once v7 is released) would be the way to go imo

@brophdawg11 brophdawg11 marked this pull request as ready for review September 5, 2023 20:13
@MichaelDeBoey MichaelDeBoey changed the title Align types with React Router feat!: align types with React Router Sep 5, 2023
@MichaelDeBoey MichaelDeBoey added BREAKING CHANGE This change will require a major version bump renderer:react labels Sep 5, 2023
@brophdawg11
Copy link
Contributor Author

Unsure what's going on in CI - but worried it might be a weird node_modules cache issue. Maybe closing/re-opening will clear something up...

Screenshot 2023-09-05 at 4 40 48 PM

@brophdawg11 brophdawg11 closed this Sep 5, 2023
@brophdawg11 brophdawg11 reopened this Sep 5, 2023
.changeset/align-rr-types.md Outdated Show resolved Hide resolved
packages/remix-react/components.tsx Outdated Show resolved Hide resolved
packages/remix-server-runtime/reexport.ts Outdated Show resolved Hide resolved
packages/remix-react/data.ts Show resolved Hide resolved
packages/remix-react/routeModules.ts Show resolved Hide resolved
packages/remix-server-runtime/data.ts Show resolved Hide resolved
packages/remix-server-runtime/data.ts Show resolved Hide resolved
packages/remix-server-runtime/routeModules.ts Show resolved Hide resolved
.changeset/align-rr-types.md Show resolved Hide resolved
packages/remix-react/components.tsx Outdated Show resolved Hide resolved
@brophdawg11
Copy link
Contributor Author

CI failure seems to be due to shelljs/shelljs#1133

brophdawg11 and others added 2 commits September 6, 2023 12:56
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
@brophdawg11 brophdawg11 merged commit b1dcc25 into release-next Sep 6, 2023
@brophdawg11 brophdawg11 deleted the brophdawg11/align-rr-types branch September 6, 2023 17:39
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

🤖 Hello there,

We just published version 2.0.0-pre.8 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

github-actions bot commented Sep 7, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-1a57073-20230907 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

github-actions bot commented Sep 7, 2023

🤖 Hello there,

We just published version 2.0.0-pre.9 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

github-actions bot commented Sep 8, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-1fac238-20230908 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 2.0.0-pre.10 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-3646f91-20230914 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!

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