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

Feature/ Helia React Vite Example #16

Merged
merged 11 commits into from
Mar 27, 2023

Conversation

polus-arcticus
Copy link
Contributor

Thought I'd try writing an example for Vite+React

@polus-arcticus polus-arcticus marked this pull request as ready for review March 19, 2023 14:01
@polus-arcticus polus-arcticus changed the title [WIP] Feature/ Helia React Vite Example Feature/ Helia React Vite Example Mar 19, 2023
Copy link
Contributor

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

LGTM, minor comments nits.

One thing I didn't post earlier is that imports should have newline between them and first line of code in the file.. but that's not a big deal. Readability is, but I don't think that's gonig to cause any future viewers issues, and we should have a lint rule set up if we want to enforce any line spacing.

examples/helia-vite-react/README.md Outdated Show resolved Hide resolved
} else {
console.log('please wait for helia to start')
}
}, [error, starting, cid, helia]) // make sure to add helia in the dependencies array of the fs will suspend
Copy link
Contributor

Choose a reason for hiding this comment

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

grammatical error on the inline comment here makes it hard to understand. is it supposed to be something like

Suggested change
}, [error, starting, cid, helia]) // make sure to add helia in the dependencies array of the fs will suspend
}, [error, starting, cid, helia, fs]) // make sure to add helia in the dependencies array or helia fs will be unresponsive

also, we should include all of the dependencies that are used within the callback

Copy link
Contributor Author

@polus-arcticus polus-arcticus Mar 22, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@polus-arcticus polus-arcticus Mar 22, 2023

Choose a reason for hiding this comment

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

  }, [error, starting, cid, helia, fs])
  // If one forgets to add helia in the dependency array in commitText, additions to the blockstore will not be picked up by react, leading to operations on fs to hang indefinitely in the generator <suspend> state. As such it would be good practice to ensure to include helia inside the dependency array of all hooks to tell react that the useCallback needs the most up to date helia state

  return { cidString, commitedText, commitText, fetchCommitedText }

Copy link
Contributor

Choose a reason for hiding this comment

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

that's a lot more context, above and beyond; thanks.

examples/helia-vite-react/src/hooks/useCommitText.jsx Outdated Show resolved Hide resolved
@polus-arcticus
Copy link
Contributor Author

Thanks for the comments @SgtPooki 🎉 I'll make these changes

Copy link
Contributor

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

LGTM, but the comment about helia dep should be enough. Can you remove the extra gifs?

@polus-arcticus
Copy link
Contributor Author

LGTM, but the comment about helia dep should be enough. Can you remove the extra gifs?

you bet, i probably just could've uploaded them to imgur or something

@SgtPooki SgtPooki merged commit 76e926d into ipfs-examples:main Mar 27, 2023
@SgtPooki
Copy link
Contributor

@polus-arcticus thanks so much for the contribution!! If you want to help out more, there's a group of prioritized examples at ipfs/helia#43! just ping me on that issue if you want to grab one and I can put your name next to it so we can make sure any other contributors aren't stepping on toes.

@polus-arcticus
Copy link
Contributor Author

@polus-arcticus thanks so much for the contribution!! If you want to help out more, there's a group of prioritized examples at ipfs/helia#43! just ping me on that issue if you want to grab one and I can put your name next to it so we can make sure any other contributors aren't stepping on toes.

Thanks @SgtPooki ! okay sounds good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants