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/farcaster feed #284

Merged
merged 45 commits into from
Jul 5, 2023
Merged

Conversation

jordanlesich
Copy link
Collaborator

@jordanlesich jordanlesich commented Jun 27, 2023

Description

Farcaster feed in Builder UI.

Motivation & context

  • Allows user to see the DAO discusson using Farcaster.
  • Lays the groundwork for a fully featured read/write model

#270

Code review

  • I would have preferred to fetch all profiles in one batch. However, in order to use the next API + useSWR + cache each individual profile separately, I ended up calling the fetch within the card.
  • I make a lot of fetches per profile (3). Unfortunately, this package doesn't allow full profile fetches. This is mitigated with SWR caching.
  • I removed the symlink for relativeTime from package.json. Shouldn't show up there anymore. relativeTime package is plugin that is already included in dayjs.
  • I added the neverthrow package. @farcaster/hub-nodejs uses neverthrow which returns 'maybe monads' instead of throwing errors. Needed this package to deal with these types. It's only 1.9kb.
    https://bundlephobia.com/package/neverthrow@6.0.0

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have done a self-review of my own code
  • Any new and existing tests pass locally with my changes
  • My changes generate no new warnings (lint warnings, console warnings, etc)

@vercel
Copy link

vercel bot commented Jun 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nouns-builder 🔄 Building (Inspect) Jul 4, 2023 5:16am
nouns-builder-mainnet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 4, 2023 5:16am
nouns-builder-testnet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 4, 2023 5:16am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
testnet-nouns-builder ⬜️ Ignored (Inspect) Jul 4, 2023 5:16am

@vercel
Copy link

vercel bot commented Jun 27, 2023

@jordanlesich is attempting to deploy a commit to the Zora Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can we remove className here instead of commenting out

turbo.json Outdated
@@ -37,6 +37,7 @@
"NEXT_PUBLIC_IPFS_GATEWAY",
"NEXT_PUBLIC_SENTRY_DSN",
"NEXT_PUBLIC_ZORA_API_KEY",
"NEXT_FARCASTER_HUB",
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need this exposed in the browser? if that's the case you need to prefix with NEXT_PUBLIC otherwise you don't need the NEXT prefix if it's just needed server side.

https://nextjs.org/docs/pages/building-your-application/configuring/environment-variables#bundling-environment-variables-for-the-browser

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep. I can do that.

import { CACHE_TIMES } from 'src/constants/cacheTimes'
import { getFarcasterProfile } from 'src/data/farcaster/queries/farcasterProfile.ts'

const handler = async (req: NextApiRequest, res: NextApiResponse) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we remove this api route now that we're fetching this data on the initial feed query?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For sure!

@@ -161,6 +169,8 @@ export const getServerSideProps: GetServerSideProps = async ({
`public, s-maxage=${maxAge}, stale-while-revalidate=${swr}`
)

getFarcasterProfile(3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need this call to getFarcasterProfile here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops!

@neokry neokry merged commit aa4a20a into ourzora:main Jul 5, 2023
0 of 5 checks passed
Copy link

vercel bot commented Mar 19, 2024

Deployment failed with the following error:

Creating the Deployment Timed Out.

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

3 participants