react: add useProfilePrice hook #868
Conversation
🦋 Changeset detectedLatest commit: c91e6a2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
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 |
|
This pull request has been linked to 1 task:
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
krzysu
force-pushed
the
T-19450/useprofileprice
branch
from
ec15aec
to
47a13ad
Compare
krzysu
force-pushed
the
T-19450/useprofileprice
branch
from
47a13ad
to
5a2a762
Compare
krzysu
force-pushed
the
T-19450/useprofileprice
branch
from
5a2a762
to
2ddec5e
Compare
krzysu
force-pushed
the
T-19450/useprofileprice
branch
from
2ddec5e
to
bd76b98
Compare
packages/gated-content/package.json
Outdated
| @@ -116,12 +116,5 @@ | |||
| "web/index.ts" | |||
| ], | |||
| "exports": true | |||
| }, | |||
| "pnpm": { | |||
There was a problem hiding this comment.
pnpm was complaining about this, it should be defined at the root of the monorepo
There was a problem hiding this comment.
I think the intent was to not have this warning screaming at people using pnpm to install these packages.
There was a problem hiding this comment.
Actually I don't know if this is effective if done on what, from the consumer perspective, is a dependency package.json
There was a problem hiding this comment.
I removed this as pnpm complains, when running install with:
packages/gated-content | WARN The field "pnpm" was found in /lens-sdk/packages/gated-content/package.json.
This will not take effect. You should configure "pnpm" at the root of the workspace instead.
| import { PromiseResultPresenter } from '../../transactions/adapters/PromiseResultPresenter'; | ||
| import { ProfilePriceGateway } from './ProfilePriceGateway'; | ||
|
|
||
| export type ProfilePriceResult = ProfilePrices; |
There was a problem hiding this comment.
Do we need to alias it? The Result suffix could lead to confusion given the term is widely used in the context of Result<T, E>.
There was a problem hiding this comment.
ProfilePrices seems good to go out in the wild, it's DTO anyway.
|
|
||
| void fetchData(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
Should we just make it lazy at this point? data comes back as ProfilePriceResult as well as part of the lazy callback.
There was a problem hiding this comment.
made it lazy, added also error handling
| * @category Misc | ||
| * @group Hooks | ||
| */ | ||
| export function useProfilePrice(): ReadResult<ProfilePriceResult> { |
There was a problem hiding this comment.
Should we consolidate the use of plural/singular? Sometimes we say ProfilePrices, here it's singular.
There was a problem hiding this comment.
I made it plural as we expect to add more currencies soon
krzysu
force-pushed
the
T-19450/useprofileprice
branch
from
580d7a5
to
4104a3b
Compare
| useEffect(() => { | ||
| void execute(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
the only problem is here, if execute is provided in useEffect deps, then the app is stuck in a never-ending loop.
There was a problem hiding this comment.
Can we brainstorm some options here? Seems a flaw in the way useDeferredTask is implemented.
There was a problem hiding this comment.
even if not perfect, I'd merge this branch as an example of onchain read hook, and keep it low profile, wdyt?
krzysu
force-pushed
the
T-19450/useprofileprice
branch
from
4104a3b
to
822be70
Compare
krzysu
force-pushed
the
T-19450/useprofileprice
branch
from
822be70
to
06a56cb
Compare
krzysu
force-pushed
the
T-19450/useprofileprice
branch
from
06a56cb
to
7cf5f17
Compare
krzysu
force-pushed
the
T-19450/useprofileprice
branch
from
7cf5f17
to
34f4a41
Compare
krzysu
force-pushed
the
T-19450/useprofileprice
branch
from
34f4a41
to
c91e6a2
Compare
|
Parking this one for now. |
No description provided.