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-react): add useRouteData hook #1281

Closed
wants to merge 3 commits into from
Closed

feat(remix-react): add useRouteData hook #1281

wants to merge 3 commits into from

Conversation

sergiodxa
Copy link
Member

In Remix Utils I created this useRouteData hook which uses the useMatches hook to get the data of any route in the current URL

The way it works is you pass the route path and you get the data back, but if the route is a layout the path may be repeated, in that case I had to ask the devs to define an id in the handle export to search using that.

This PR moves this useRouteData hook and taking advantage of being able to access the internal context of Remix you can with this implementation pass the route ID (the file name without the extension), which is easy to know for the developer.


Why is this hook useful?

A super common requirement is to access the data of the parent route inside the child route, most people put that in the <Outlet context /> but it makes no sense when Remix already have the data of the loader inside a context, using useMatches is a way to solve it in user-land but this hook has a simpler implementation and could be used out of the box.

And, contrarian to the Outlet solution, you can also access a child route data from a parent one, so you can communicate data in both directions from top to bottom or from bottom to top.

@kyleharper
Copy link

I built this hook yesterday.

+1 for getting it into the Remix Utils 👍

@sergiodxa
Copy link
Member Author

I built this hook yesterday.

+1 for getting it into the Remix Utils 👍

This is already in Remix Utils, this PR is to add it directly in Remix which is better because it can use the internal context.

@kentcdodds
Copy link
Member

Another reason <Outlet context /> isn't suitable for loader data is because you could have a situation where route A puts something into Outlet context which route C depends upon. Then later someone adds context to route B (which is between the two) and surprisingly/unexpectedly breaks route C.

I'm in favor of this improvement.

@zachdtaylor
Copy link
Contributor

+1, this would be awesome to have as part of Remix itself.

@ryanflorence
Copy link
Member

ryanflorence commented Feb 11, 2022

We only ever intended useMatches for use cases where layouts needed child route data to build things like breadcrumbs.

I'm not sure why we're so bothered by child routes accessing parent data with it 🙃

useMatches as an access point for "all route data" is dang convenient though, and this hook makes it even more convenient. When you make something more convenient it gets used more! So I've been hesitant here. This whole thing reminds me of when we created React Broadcast and the React team was like "yeah, that's nice but it makes using context too convenient", and then they implemented it directly into React as React.createContext() the next release 🤣

I've already made my own useRootData() and useParentData() hooks around useMatches in my Remix apps because it really is just so nice. I know it's not how we intended useMatches to be used, but in practice, it's turning out to be a really great way to share route data across the hierarchy.

@mjackson I want to get your thumbs up on this, but I think we should add it.

I'd change this to just use route ids to access the data. We already exposed route ids as public API in parentsData of the meta function.

export function meta({ parentsData }) {
  let { user } = parentsData["root"];
  let whatever = parentsData["routes/whatever"];
  // ...
}

This hook is not any different:

export function Comp() {
  let { user } = useRouteData("root");
  let whatever = useRouteData("routes/whatever");
  // ...
}

It's far more convenient than <Outlet context /> because you don't have to keep drilling the values down (and augmenting with the new data) at each level. Additionally, if Remix wasn't the one doing the fetching, your app code would already have access to all this data. It's not like we're leaking implementation details either, the app defined the routes and supplied the data, we would just be exposing those app-supplied values with app-defined ids.

@sergiodxa if @mjackson gives the thumbs up, I'd like to see the hook just accept route ids and not require the handle or url paths.

@kentcdodds
Copy link
Member

I've also put a lot of thought into this and agree we should have this feature with the route ID as mentioned. We just need to take care in the docs to explain the trade-offs so people don't use it unnecessarily.

@sergiodxa
Copy link
Member Author

@sergiodxa if @mjackson gives the thumbs up, I'd like to see the hook just accept route ids and not require the handle or url paths.

The hook of this PR uses the route ID, the hook of Remix Utils doesn’t because it was not possible.

@andresgutgon
Copy link

+1 niceee

docs/api/remix.md Outdated Show resolved Hide resolved
@bluefire2121
Copy link

+1 for this feature. It would help when sending data to a resource route whose parent already has that data. Right now, one would have to use the loader hook to request the data twice: once for the parent route and once for the resource route.

@machour
Copy link
Collaborator

machour commented Mar 17, 2022

Right now, one would have to use the loader hook to request the data twice:

You can already use useMatches() and avoid requesting the data again.

@Mange
Copy link

Mange commented Mar 18, 2022

Right now, one would have to use the loader hook to request the data twice [in a resource route]:

You can already use useMatches() and avoid requesting the data again.

useMatches can only be used in a React context, not inside a loader function. Resource routes are only a loader function.

It would help when sending data to a resource route whose parent already has that data.

Would it? This is still a hook, isn't it?

@sergiodxa
Copy link
Member Author

+1 for this feature. It would help when sending data to a resource route whose parent already has that data. Right now, one would have to use the loader hook to request the data twice: once for the parent route and once for the resource route.

This will not solve that, because this is a React hook, it'll make it easier to get another route data on React but server-side you still need to get the same data on each loader it's needed.

@machour
Copy link
Collaborator

machour commented Mar 20, 2022

@sergiodxa Can you fix conflicts pretty please?

sergiodxa and others added 3 commits March 21, 2022 10:27
@sergiodxa sergiodxa requested a review from machour March 21, 2022 15:52
@sergiodxa
Copy link
Member Author

@machour done

This hook returns the JSON parsed data from the route loader function of any matching route in the current URL.

```tsx filename=app/routes/invoices.tsx
export function loader() {
Copy link
Member

Choose a reason for hiding this comment

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

Since we're in a TS file

Suggested change
export function loader() {
export const loader: LoaderFunction = () => {


```tsx filename=app/routes/invoices.tsx
export function loader() {
return fakeDb.invoices.findAll();
Copy link
Member

Choose a reason for hiding this comment

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

action & loader need to return Response or use json helper

Suggested change
return fakeDb.invoices.findAll();
return json(await fakeDb.invoices.findAll());

Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sold on this requirement. I know we already merged some docs changes that said we have to do this, but I'd like to find a way to still allow people to return whatever data they want and we just encode/decode for them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a lot of the confusion about typing loader return value and useLoaderData have to do with Remix allowing you to return a naked object from the loader.

I strongly recommend deprecating this and having them return an actual Response. That way there is no magic that leads to misunderstanding. They will know that loaders always return a Response.

Copy link
Member

Choose a reason for hiding this comment

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

I'm also in favour of merging @chaance's #2252 and making a non-Response return deprecated

@@ -0,0 +1,22 @@
import type { LoaderFunction } from "remix";
import { Outlet, useRouteData } from "remix";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { Outlet, useRouteData } from "remix";
import { json, Outlet, useRouteData } from "remix";

import { Outlet, useRouteData } from "remix";

export let loader: LoaderFunction = async () => {
return { title: "Parent Route Data" };
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return { title: "Parent Route Data" };
return json({ title: "Parent Route Data" });

@@ -0,0 +1,17 @@
import type { LoaderFunction } from "remix";
import { useRouteData } from "remix";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { useRouteData } from "remix";
import { json, useRouteData } from "remix";

import { useRouteData } from "remix";

export let loader: LoaderFunction = async () => {
return { title: "Child Route Data" };
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return { title: "Child Route Data" };
return json({ title: "Child Route Data" });


afterEach(() => browser.close());

describe("the parent should render the child title", () => {
Copy link
Member

Choose a reason for hiding this comment

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

As @kentcdodds also requested in #2411 (comment)

I think we’re trying to use Ryan’s createFixture utility as much as possible instead of the gist app. Could you switch this test to use that instead?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. We are trying to get rid of the gists app for testing stuff and use a separate createFixture test instead. Thanks!

@MichaelDeBoey MichaelDeBoey changed the title Add useRouteData hook feat(remix-react): add useRouteData hook Mar 22, 2022
@bluefire2121
Copy link

+1 for this feature. It would help when sending data to a resource route whose parent already has that data. Right now, one would have to use the loader hook to request the data twice: once for the parent route and once for the resource route.

This will not solve that, because this is a React hook, it'll make it easier to get another route data on React but server-side you still need to get the same data on each loader it's needed.

Thanks @sergiodxa and @Mange for the reminder. You all are both correct. Hooks have to top level.

@chaance
Copy link
Collaborator

chaance commented Apr 29, 2022

Closing this not because we don't want the feature, but because we'll be introducing it into React Router instead (as well as many other Remix APIs). Stay tuned, and thanks @sergiodxa for the work here!

@chaance chaance closed this Apr 29, 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.

None yet