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

fix/load hypercerts from root #13

Closed
wants to merge 1 commit into from
Closed

Conversation

CJ-Rose
Copy link
Collaborator

@CJ-Rose CJ-Rose commented Jan 26, 2024

No description provided.

@CJ-Rose
Copy link
Collaborator Author

CJ-Rose commented Jan 26, 2024

Remix gives us the useRouteLoaderData hook to pull data into a route (_index.tsx) that was loaded in a different one (impact-reports.tsx). But because impact-reports route is never actually hit, the api is not called and data is not loaded. Proposed solution is to move HC loading into root.tsx as that route is always active. In this branch _index.tsx is able to display all contents of the updated Report object. 🥳 🙏 thank you @baumstern

also please check out the failing test, i can't see how my changes would have caused it to fail ??

@baumstern
Copy link
Contributor

thanks, just a few suggestions:

  1. it's okay to overlook the failing test, they've removed in feat: retrieve impact reports #10. @thebeyondr and me had a call and decided to keep only the test against server functionality

  2. it seems like loader is designed to be part of each route, providing data to the route component (https://remix.run/docs/en/main/discussion/data-flow#route-loader). A good practice would be to defineloader directly within app/routes/_index.tsx. Then, you can use the useLoaderData hook to access the loader data from your component

  3. I think PR should be opened against main branch

@CJ-Rose
Copy link
Collaborator Author

CJ-Rose commented Jan 26, 2024

it seems like loader is designed to be part of each route, providing data to the route component (https://remix.run/docs/en/main/discussion/data-flow#route-loader). A good practice would be to define loader directly within app/routes/_index.tsx. Then, you can use the useLoaderData hook to access the loader data from your component

@baumstern right, each route can have its own loader. maybe i misunderstood but i thought we were trying to load HC data independently of _index.tsx and allow multiple routes to use it. but yeah it can also be moved into _index.tsx and other routes can retrieve from there. @thebeyondr seems this is a consideration for caching approach?

@thebeyondr thebeyondr added the feature 🚀 New feature or request label Jan 26, 2024
Copy link
Collaborator

@thebeyondr thebeyondr left a comment

Choose a reason for hiding this comment

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

  • think you can remove .DS_Store and add it to our gitignore.
  • useRouteLoader can use any loader based on its route, including "root"! as per here
  • seems your commit failed a check, please investigate it

Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

answer here

it wasn't tracked or added to my file structure. now i know 🙃

Copy link
Collaborator

Choose a reason for hiding this comment

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

lol apple has built in trolling 💀

import "./tailwind.css";

export const loader = async ({ request }: LoaderFunctionArgs) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

fetch /impact-reports instead - which does basically what this function does

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the intention is to use the loader in root instead of impact-reports.tsx, and delete that file.
maybe i am still missing a piece but i am not able to get data into _index route from impact-reports route. using fetch(/impact-reports) doesn't trigger its loader. with the code from impact-reports.tsx moved into root.tsx i am able to use the data in _index route because the loader inroot is already active.

Copy link
Collaborator

@thebeyondr thebeyondr Jan 27, 2024

Choose a reason for hiding this comment

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

somehow missed this but I gave it a shot just now and I ran into the same issue! Turns out just fetching /impact-reports won't work as its an incomplete URL. Here's what worked (in _index.tsx):

export const loader = async ({ request }: LoaderFunctionArgs) => {
	const url = new URL(request.url);
	const response = await fetch(`${url.protocol}//${url.host}/impact-reports`);
	const data = await response.json();
	return json(data);
};

This allows the fetch to work dynamically on localhost and when its deployed.

proof:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

const response = await fetch(${url.protocol}//${url.host}/impact-reports);

cool ✨
did you find this referenced in remix docs somewhere? i couldn't find it. i had tried hardcoding the full path couldn't get it to resolve.

};

// custom hook to pass the loader's type to other routes
export function useRootLoaderData() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need to export a custom hook. in any other route just use useRouteLoaderData<typeof loader>("root") and it will work. this custom hook would cause confusion as it hijacks the original hook.

@thebeyondr
Copy link
Collaborator

2. it seems like loader is designed to be part of each route, providing data to the route component (https://remix.run/docs/en/main/discussion/data-flow#route-loader). A good practice would be to defineloader directly within app/routes/_index.tsx. Then, you can use the useLoaderData hook to access the loader data from your component

A loader can be in a route, and when it is, any useLoaderData hook can reference any other loader via the route its in! So I can have a loader in /cats and in /vets i can still use the data from the cat loader with useLoaderData("cats")!

@CJ-Rose
Copy link
Collaborator Author

CJ-Rose commented Jan 26, 2024

seems your commit failed a check, please investigate it

@thebeyondr as daehyun noted above, the test has been removed in #10.
i was working from the backend branch, which i guess was not updated with latest changes to the PR..
how should i access the current branch of the PR to work from?

@thebeyondr
Copy link
Collaborator

thebeyondr commented Jan 26, 2024

i was working from the backend branch, which i guess was not updated with latest changes to the PR..\nhow should i access the current branch of the PR to work from?

I see. The backend branch is new to me. I think you started working from something that is also in progress. You can continue to make the additional changes on your end and when #10 is merged to main, you can merge main into this branch and then merge wherever from there

@CJ-Rose
Copy link
Collaborator Author

CJ-Rose commented Jan 26, 2024

I see. The backend branch is new to me. You may be better served keeping up with the main branch if you're uncertain. The hc stuff has not been merged into main yet. You may follow the progress here: #10

yes i have been following the PR. but i don't understand how i can work from it, other than cloning the fork and branching from there. which seems like something we would want to avoid??

When I say "to work from it", I mean add features based on the PR. I wouldn't advise this until the PR is merged.

If you're trying to review a PR or update the existing feature being proposed, vscode/cursor allows you to pull PRs and the github cli offers a command for it.

We can do a call if you think that would be helpful

@thebeyondr
Copy link
Collaborator

I don't usually work from PRs as they are volatile. In this case, I'd wait until merges are complete or I've spoken to the person responsible for the pr for a timeline for merging their work so I can do mine

@CJ-Rose
Copy link
Collaborator Author

CJ-Rose commented Jan 26, 2024

ok. but as i am being asked to review and approve the contents of #10, i was trying to see if i could get it to deliver the data it is supposed to be retrieving.

@thebeyondr
Copy link
Collaborator

Gotcha. That context was entirely lost on me as this PR seems to be proposing a use for what is in #10, not a test of its functionality.

@CJ-Rose
Copy link
Collaborator Author

CJ-Rose commented Jan 26, 2024

If you're trying to review a PR or update the existing feature being proposed, vscode/cursor allows you to pull PRs and the github cli offers a command for it.

yes i was trying to suggest updates to the existing feature because i don't think the impact-reports route can share its data with another route as it is currently implemented. i can try again after it is merged as it seems ready otherwise.

this is pretty cool ✨✨ vsCode extension for github PRs

@thebeyondr
Copy link
Collaborator

@CJ-Rose @baumstern thinking more about this and I think the loader should be moved to the _index route. The return of /impact-reports yield all reports, something not needed for /reports/:id.

While the implementation is allowed in Remix, I don't think its right for our use case.

@baumstern
Copy link
Contributor

PR #10 has been merged into the main, so we no longer need the backend branch. Could you please rebase this PR on the latest main branch and change the target branch from backend to main? @CJ-Rose

You can use this commands to rebase PR:

git fetch
git switch fix/load-HC-from-root
git reset --hard 0f21312
git cherry-pick 83cfed2074378863378269a63e1a0a7dd7c0415a

@CJ-Rose
Copy link
Collaborator Author

CJ-Rose commented Jan 29, 2024

@baumstern @thebeyondr closing this PR and removing backend and fix/load-HC-from-root branches. will make a fresh branch to move loader for all reports into _index route.

@CJ-Rose CJ-Rose closed this Jan 29, 2024
@CJ-Rose CJ-Rose deleted the fix/load-HC-from-root branch January 29, 2024 18:43
@CJ-Rose CJ-Rose mentioned this pull request Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🚀 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants