Skip to content

Commit

Permalink
[SDK] Update to NFT Components (#5655)
Browse files Browse the repository at this point in the history
CNCT-2584

- add custom resolver methods
- add better test coverage
- add caching for the getNFT method
- small fix: handling falsy values

<!-- start pr-codex -->

---

## PR-Codex overview
This PR focuses on enhancing the NFT components in the `thirdweb` library by improving functionality, performance, and test coverage. It introduces custom resolver methods and caching mechanisms, along with fixes for handling falsy values.

### Detailed summary
- Added custom resolver methods to `NFTMedia`, `NFTName`, and `NFTDescription`.
- Implemented caching for the NFT info getter to improve performance.
- Fixed handling of falsy values for NFT media `src`, `name`, and `description`.
- Enhanced test coverage by extracting internal logic for testing.

> ✨ Ask PR-Codex anything about this PR by commenting with `/codex {your question}`

<!-- end pr-codex -->
  • Loading branch information
kien-ngo committed Dec 9, 2024
1 parent f680496 commit f69d1aa
Show file tree
Hide file tree
Showing 12 changed files with 626 additions and 137 deletions.
9 changes: 9 additions & 0 deletions .changeset/serious-bananas-boil.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"thirdweb": minor
---

Improve NFT Components
- Add custom resolver methods to NFTMedia, NFTName and NFTDescription
- Add caching for the NFT-info-getter method to improve performance
- Small fix to handle falsy values for NFT media src, name and description
- Improve test coverage by extracting internal logics and testing them
1 change: 1 addition & 0 deletions packages/thirdweb/src/exports/react.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ export {
export {
NFTMedia,
type NFTMediaProps,
type NFTMediaInfo,
} from "../react/web/ui/prebuilt/NFT/media.js";

export { useConnectionManager } from "../react/core/providers/connection-manager.js";
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import { describe, expect, it } from "vitest";
import {
DOODLES_CONTRACT,
DROP1155_CONTRACT,
UNISWAPV3_FACTORY_CONTRACT,
} from "~test/test-contracts.js";
import { fetchNftDescription } from "./description.js";

describe.runIf(process.env.TW_SECRET_KEY)("NFTDescription", () => {
it("fetchNftDescription should work with ERC721", async () => {
const desc = await fetchNftDescription({
contract: DOODLES_CONTRACT,
tokenId: 0n,
});
expect(desc).toBe(
"A community-driven collectibles project featuring art by Burnt Toast. Doodles come in a joyful range of colors, traits and sizes with a collection size of 10,000. Each Doodle allows its owner to vote for experiences and activations paid for by the Doodles Community Treasury. Burnt Toast is the working alias for Scott Martin, a Canadian–based illustrator, designer, animator and muralist.",
);
});

it("fetchNftDescription should work with ERC1155", async () => {
const desc = await fetchNftDescription({
contract: DROP1155_CONTRACT,
tokenId: 0n,
});
expect(desc).toBe("");
});

it("fetchNftDescription should respect descriptionResolver as a string", async () => {
const desc = await fetchNftDescription({
contract: DOODLES_CONTRACT,
tokenId: 0n,
descriptionResolver: "string",
});
expect(desc).toBe("string");
});

it("fetchNftDescription should respect descriptionResolver as a non-async function", async () => {
const desc = await fetchNftDescription({
contract: DOODLES_CONTRACT,
tokenId: 0n,
descriptionResolver: () => "non-async",
});
expect(desc).toBe("non-async");
});

it("fetchNftDescription should respect descriptionResolver as a async function", async () => {
const desc = await fetchNftDescription({
contract: DOODLES_CONTRACT,
tokenId: 0n,
descriptionResolver: async () => "async",
});
expect(desc).toBe("async");
});

it("fetchNftDescription should throw error if failed to resolve nft info", async () => {
await expect(() =>
fetchNftDescription({
contract: UNISWAPV3_FACTORY_CONTRACT,
tokenId: 0n,
}),
).rejects.toThrowError("Failed to resolve NFT info");
});
});
81 changes: 70 additions & 11 deletions packages/thirdweb/src/react/web/ui/prebuilt/NFT/description.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
"use client";

import type { UseQueryOptions } from "@tanstack/react-query";
import { type UseQueryOptions, useQuery } from "@tanstack/react-query";
import type { JSX } from "react";
import type { NFT } from "../../../../../utils/nft/parseNft.js";
import { useNftInfo } from "./hooks.js";
import type { ThirdwebContract } from "../../../../../contract/contract.js";
import { useNFTContext } from "./provider.js";
import { getNFTInfo } from "./utils.js";

export interface NFTDescriptionProps
extends Omit<React.HTMLAttributes<HTMLSpanElement>, "children"> {
Expand All @@ -13,7 +13,12 @@ export interface NFTDescriptionProps
/**
* Optional `useQuery` params
*/
queryOptions?: Omit<UseQueryOptions<NFT>, "queryFn" | "queryKey">;
queryOptions?: Omit<UseQueryOptions<string>, "queryFn" | "queryKey">;
/**
* This prop can be a string or a (async) function that resolves to a string, representing the description of the NFT
* This is particularly useful if you already have a way to fetch the data.
*/
descriptionResolver?: string | (() => string) | (() => Promise<string>);
}

/**
Expand Down Expand Up @@ -58,6 +63,21 @@ export interface NFTDescriptionProps
* </NFTProvider>
* ```
*
* ### Override the description with the `descriptionResolver` prop
* If you already have the url, you can skip the network requests and pass it directly to the NFTDescription
* ```tsx
* <NFTDescription descriptionResolver="The desc of the NFT" />
* ```
*
* You can also pass in your own custom (async) function that retrieves the description
* ```tsx
* const getDescription = async () => {
* // ...
* return description;
* };
*
* <NFTDescription descriptionResolver={getDescription} />
* ```
* @component
* @nft
* @beta
Expand All @@ -66,22 +86,61 @@ export function NFTDescription({
loadingComponent,
fallbackComponent,
queryOptions,
descriptionResolver,
...restProps
}: NFTDescriptionProps) {
const { contract, tokenId } = useNFTContext();
const nftQuery = useNftInfo({
contract,
tokenId,
queryOptions,
const descQuery = useQuery({
queryKey: [
"_internal_nft_description_",
contract.chain.id,
tokenId.toString(),
{
resolver:
typeof descriptionResolver === "string"
? descriptionResolver

Check warning on line 101 in packages/thirdweb/src/react/web/ui/prebuilt/NFT/description.tsx

View check run for this annotation

Codecov / codecov/patch

packages/thirdweb/src/react/web/ui/prebuilt/NFT/description.tsx#L101

Added line #L101 was not covered by tests
: typeof descriptionResolver === "function"
? descriptionResolver.toString()

Check warning on line 103 in packages/thirdweb/src/react/web/ui/prebuilt/NFT/description.tsx

View check run for this annotation

Codecov / codecov/patch

packages/thirdweb/src/react/web/ui/prebuilt/NFT/description.tsx#L103

Added line #L103 was not covered by tests
: undefined,
},
],
queryFn: async (): Promise<string> =>
fetchNftDescription({ descriptionResolver, contract, tokenId }),
...queryOptions,
});

if (nftQuery.isLoading) {
if (descQuery.isLoading) {
return loadingComponent || null;
}

if (!nftQuery.data?.metadata?.description) {
if (!descQuery.data) {

Check warning on line 116 in packages/thirdweb/src/react/web/ui/prebuilt/NFT/description.tsx

View check run for this annotation

Codecov / codecov/patch

packages/thirdweb/src/react/web/ui/prebuilt/NFT/description.tsx#L116

Added line #L116 was not covered by tests
return fallbackComponent || null;
}

return <span {...restProps}>{nftQuery.data.metadata.description}</span>;
return <span {...restProps}>{descQuery.data}</span>;
}

Check warning on line 121 in packages/thirdweb/src/react/web/ui/prebuilt/NFT/description.tsx

View check run for this annotation

Codecov / codecov/patch

packages/thirdweb/src/react/web/ui/prebuilt/NFT/description.tsx#L120-L121

Added lines #L120 - L121 were not covered by tests

/**
* @internal Exported for tests
*/
export async function fetchNftDescription(props: {
descriptionResolver?: string | (() => string) | (() => Promise<string>);
contract: ThirdwebContract;
tokenId: bigint;
}): Promise<string> {
const { descriptionResolver, contract, tokenId } = props;
if (typeof descriptionResolver === "string") {
return descriptionResolver;
}
if (typeof descriptionResolver === "function") {
return descriptionResolver();
}
const nft = await getNFTInfo({ contract, tokenId }).catch(() => undefined);
if (!nft) {
throw new Error("Failed to resolve NFT info");
}
if (typeof nft.metadata.description !== "string") {
throw new Error("Failed to resolve NFT description");

Check warning on line 143 in packages/thirdweb/src/react/web/ui/prebuilt/NFT/description.tsx

View check run for this annotation

Codecov / codecov/patch

packages/thirdweb/src/react/web/ui/prebuilt/NFT/description.tsx#L143

Added line #L143 was not covered by tests
}
return nft.metadata.description;
}
53 changes: 0 additions & 53 deletions packages/thirdweb/src/react/web/ui/prebuilt/NFT/hooks.tsx

This file was deleted.

77 changes: 77 additions & 0 deletions packages/thirdweb/src/react/web/ui/prebuilt/NFT/media.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import { describe, expect, it } from "vitest";
import {
DOODLES_CONTRACT,
DROP1155_CONTRACT,
UNISWAPV3_FACTORY_CONTRACT,
} from "~test/test-contracts.js";
import { fetchNftMedia } from "./media.js";

describe.runIf(process.env.TW_SECRET_KEY)("NFTMedia", () => {
it("fetchNftMedia should work with ERC721", async () => {
const desc = await fetchNftMedia({
contract: DOODLES_CONTRACT,
tokenId: 0n,
});
expect(desc).toStrictEqual({
src: "ipfs://QmUEfFfwAh4wyB5UfHCVPUxis4j4Q4kJXtm5x5p3g1fVUn",
poster: undefined,
});
});

it("fetchNftMedia should work with ERC1155", async () => {
const desc = await fetchNftMedia({
contract: DROP1155_CONTRACT,
tokenId: 0n,
});
expect(desc).toStrictEqual({
src: "ipfs://QmeGCqV1mSHTZrvuFzW1XZdCRRGXB6AmSotTqHoxA2xfDo/1.mp4",
poster: "ipfs://QmeGCqV1mSHTZrvuFzW1XZdCRRGXB6AmSotTqHoxA2xfDo/0.png",
});
});

it("fetchNftMedia should respect mediaResolver as a string", async () => {
const desc = await fetchNftMedia({
contract: DOODLES_CONTRACT,
tokenId: 0n,
mediaResolver: {
src: "string",
poster: undefined,
},
});
expect(desc).toStrictEqual({ src: "string", poster: undefined });
});

it("fetchNftMedia should respect mediaResolver as a non-async function", async () => {
const desc = await fetchNftMedia({
contract: DOODLES_CONTRACT,
tokenId: 0n,
mediaResolver: () => ({
src: "non-async",
poster: undefined,
}),
});
expect(desc).toStrictEqual({ src: "non-async", poster: undefined });
});

it("fetchNftMedia should respect mediaResolver as a async function", async () => {
const desc = await fetchNftMedia({
contract: DOODLES_CONTRACT,
tokenId: 0n,
mediaResolver: async () =>
await {
src: "async",
poster: undefined,
},
});
expect(desc).toStrictEqual({ src: "async", poster: undefined });
});

it("fetchNftMedia should throw error if failed to resolve nft info", async () => {
await expect(() =>
fetchNftMedia({
contract: UNISWAPV3_FACTORY_CONTRACT,
tokenId: 0n,
}),
).rejects.toThrowError("Failed to resolve NFT info");
});
});
Loading

0 comments on commit f69d1aa

Please sign in to comment.