feat(spore): dob-render-sdk migration #320
Conversation
🦋 Changeset detectedLatest commit: 04a9592 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
✅ Deploy Preview for appccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for liveccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for apiccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for docsccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
/canary |
|
🚀 Canary version published successfully! View workflow run The following packages have been published to npm:
|
|
/gemini review |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the dob-render-sdk into the spore module, introducing new rendering capabilities. The changes are extensive, adding new dependencies, rendering logic, and utilities. My review focuses on improving robustness, maintainability, and cross-platform compatibility. I've identified several critical issues, including the use of browser-specific APIs in code that should be platform-agnostic and potential runtime errors due to incorrect data type assumptions. Additionally, there are high-severity issues related to error handling and dependency management, and medium-severity issues concerning code clarity, configurability, and testing practices.
| private _queryUrlFn = async (url: string) => { | ||
| const response = await fetch(url); | ||
| const blob = await response.blob(); | ||
| return new Promise<IpfsResult>((resolve, reject) => { | ||
| const reader = new FileReader(); | ||
|
|
||
| reader.onload = function () { | ||
| const base64 = this.result as string; | ||
| resolve(base64); | ||
| }; | ||
| reader.onerror = (error) => { | ||
| reject(new Error(`FileReader error: ${error.type}`)); | ||
| }; | ||
| reader.readAsDataURL(blob); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
The _queryUrlFn function uses FileReader, which is a browser-only API. This will cause the library to crash when used in a Node.js environment. To ensure this module is isomorphic (works in both browser and Node.js), you should avoid browser-specific APIs or provide a Node.js-specific implementation path.
| if (item.parsedStyle.breakLine === 0 && acc[acc.length - 1]) { | ||
| const lastEl = acc[acc.length - 1]; | ||
| el.type = "span"; | ||
| delete (el.props.style as Record<string, unknown>).width; | ||
| (el.props.style as Record<string, unknown>).display = "block"; | ||
| (lastEl.props.children as RenderElement[]).push(el); | ||
| return acc; | ||
| } |
There was a problem hiding this comment.
There is a potential runtime error here. lastEl.props.children is initialized as a string, but on line 53 it is cast to RenderElement[] and push is called on it. This will throw a TypeError because strings do not have a push method. You need to handle the case where children is a string before attempting to push to it.
if (item.parsedStyle.breakLine === 0 && acc.length > 0) {
const lastEl = acc[acc.length - 1];
el.type = "span";
delete (el.props.style as Record<string, unknown>).width;
(el.props.style as Record<string, unknown>).display = "block";
if (!Array.isArray(lastEl.props.children)) {
lastEl.props.children = [lastEl.props.children as string];
}
(lastEl.props.children as (string | RenderElement)[]).push(el);
return acc;
}| style: { | ||
| display: "flex", | ||
| width: `${width}px`, | ||
| height: `${width}px`, |
There was a problem hiding this comment.
The height of the container div is being set to ${width}px, which seems to be a typo. It should likely be set to ${height}px to match the image's height and avoid potential layout issues or cropping if the SVG is not square.
| height: `${width}px`, | |
| height: `${height}px`, |
| private _queryBtcFsFn: QueryBtcFsFn = async (uri) => { | ||
| console.log("requiring", uri); | ||
| const response = await fetch( | ||
| `https://dob-decoder.ckbccc.com/restful/dob_extract_image?uri=${uri}&encode=base64`, | ||
| ); | ||
| return { | ||
| content: await response.text(), | ||
| content_type: "", | ||
| }; | ||
| }; |
There was a problem hiding this comment.
This function has a hardcoded URL and lacks proper error handling.
- The URL
https://dob-decoder.ckbccc.comis hardcoded. It should usethis._dobDecodeServerURLto be consistent with the rest of the class and allow for configuration. - There is no error handling for the
fetchcall. If the request fails, it will throw an unhandled exception. Please add a check forresponse.ok. - The
content_typeis hardcoded to an empty string. It would be better to get this from the response headers.
private _queryBtcFsFn: QueryBtcFsFn = async (uri) => {
console.log("requiring", uri);
const response = await fetch(
`${this._dobDecodeServerURL}/restful/dob_extract_image?uri=${uri}&encode=base64`,
);
if (!response.ok) {
throw new Error(`Failed to query BtcFs: ${response.status} ${response.statusText}`);
}
return {
content: await response.text(),
content_type: response.headers.get('content-type') || "",
};
};| const response = await fetch(config.dobDecodeServerURL, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| id: 2, | ||
| jsonrpc: "2.0", | ||
| method: "dob_decode", | ||
| params: [tokenKey], | ||
| }), | ||
| }); | ||
| return response.json() as Promise<DobDecodeResponse>; |
There was a problem hiding this comment.
This function does not handle network errors or non-successful HTTP status codes from the fetch call. If the request fails, it will throw an unhandled exception. It's important to check the response status and handle errors gracefully. Additionally, the type cast as Promise<DobDecodeResponse> on response.json() is incorrect and should be removed.
const response = await fetch(config.dobDecodeServerURL, {
method: "POST",
headers: {
"Content-Type": "application/json",
},
body: JSON.stringify({
id: 2,
jsonrpc: "2.0",
method: "dob_decode",
params: [tokenKey],
}),
});
if (!response.ok) {
const errorBody = await response.text();
throw new Error(`Failed to decode DOB: ${response.status} ${response.statusText} - ${errorBody}`);
}
return response.json();| if (typeof account === "string") { | ||
| if (account.length > 10) { | ||
| fontSize = fontSize / 2; | ||
| } | ||
| if (account.length > 20) { | ||
| fontSize = fontSize / 2; | ||
| } | ||
| if (account.length > 30) { | ||
| fontSize = fontSize * 0.75; | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for adjusting fontSize uses a series of if statements that are not mutually exclusive. This makes the code hard to understand and might lead to unintended cumulative font size reductions. For example, a string of length 25 will have its font size halved twice. If these conditions are meant to be exclusive, please use an if-else if structure for clarity and correctness. If the cumulative effect is intended, please add a comment to explain it.
| const { regular, italic, bold, boldItalic } = props.font ?? { | ||
| regular: TurretRoadMediumFont, | ||
| italic: TurretRoadMediumFont, | ||
| bold: TurretRoadBoldFont, | ||
| boldItalic: TurretRoadBoldFont, | ||
| }; |
There was a problem hiding this comment.
The default font for italic is the same as regular, and boldItalic is the same as bold. This means that without providing custom fonts, italic styling will not be applied. If this is an intentional fallback because the default font lacks italic variants, it would be helpful to add a comment explaining this. Otherwise, this might be a bug.
| it("should respose a decoded dob render data from a spore id", async () => { | ||
| // The spore id that you want to decode (must be a valid spore dob) | ||
| const sporeId = | ||
| "dc19e68af1793924845e2a4dbc23598ed919dcfe44d3f9cd90964fe590efb0e4"; | ||
|
|
||
| // Decode from spore id | ||
| const dob = await renderByTokenKey(sporeId); | ||
| console.log(dob); | ||
| }, 60000); |
There was a problem hiding this comment.
This test case lacks assertions. A test should verify that the code behaves as expected, but this one only logs the result to the console. It will pass as long as no error is thrown. Please add expect assertions to validate the output of renderByTokenKey. Also, console.log should be removed from test files before merging.
| export function base64ToArrayBuffer(base64: string): ArrayBuffer { | ||
| const binaryString = atob(base64); | ||
|
|
||
| const uint8Array = new Uint8Array(binaryString.length); | ||
| for (let i = 0; i < binaryString.length; i++) { | ||
| uint8Array[i] = binaryString.charCodeAt(i); | ||
| } | ||
|
|
||
| return uint8Array.buffer; | ||
| } |
There was a problem hiding this comment.
The atob function is a legacy browser API and is not available in all JavaScript environments, particularly older versions of Node.js. To ensure this utility is isomorphic, consider using Buffer.from(base64, 'base64') for Node.js environments.
export function base64ToArrayBuffer(base64: string): ArrayBuffer {
if (typeof Buffer !== "undefined") {
return Buffer.from(base64, 'base64').buffer;
}
const binaryString = atob(base64);
const uint8Array = new Uint8Array(binaryString.length);
for (let i = 0; i < binaryString.length; i++) {
uint8Array[i] = binaryString.charCodeAt(i);
}
return uint8Array.buffer;
}| export function hexToBase64(hexstring: string): string { | ||
| const str = hexstring | ||
| .match(/\w{2}/g) | ||
| ?.map((a) => String.fromCharCode(parseInt(a, 16))) | ||
| .join(""); | ||
| return str ? btoa(str) : ""; | ||
| } |
There was a problem hiding this comment.
The btoa function is a legacy browser API and is not available in all JavaScript environments. For better cross-platform compatibility, you should use Buffer.from(str, 'binary').toString('base64') in Node.js environments.
export function hexToBase64(hexstring: string): string {
const str = hexstring
.match(/\w{2}/g)
?.map((a) => String.fromCharCode(parseInt(a, 16)))
.join("");
if (!str) {
return "";
}
if (typeof Buffer !== 'undefined') {
return Buffer.from(str, 'binary').toString('base64');
}
return btoa(str);
}
There was a problem hiding this comment.
Code Review
This pull request migrates the dob-render-sdk into the spore module, introducing new rendering capabilities. The changes are extensive, adding new dependencies and a significant amount of rendering logic. My review focuses on improving code robustness, readability, and ensuring the code is environment-agnostic (browser vs. Node.js). I've identified several critical issues related to the use of browser-specific APIs that will cause crashes in a Node.js environment, as well as high-severity issues like lack of error handling for network requests and hardcoded URLs. I've also included medium-severity suggestions to improve code quality and fix potential logic bugs.
| return new Promise<IpfsResult>((resolve, reject) => { | ||
| const reader = new FileReader(); | ||
|
|
||
| reader.onload = function () { | ||
| const base64 = this.result as string; | ||
| resolve(base64); | ||
| }; | ||
| reader.onerror = (error) => { | ||
| reject(new Error(`FileReader error: ${error.type}`)); | ||
| }; | ||
| reader.readAsDataURL(blob); | ||
| }); |
There was a problem hiding this comment.
The FileReader API is only available in browsers and will cause this code to crash in a Node.js environment. To ensure this library is isomorphic (runs in both browser and Node.js), you should provide a Node.js-compatible implementation for fetching and base64-encoding the content.
if (typeof window !== 'undefined' && typeof FileReader !== 'undefined') {
// Browser environment
return new Promise<IpfsResult>((resolve, reject) => {
const reader = new FileReader();
reader.onload = function () {
const dataUrl = this.result as string;
resolve(dataUrl);
};
reader.onerror = (error) => {
reject(new Error(`FileReader error: ${error.type}`));
};
reader.readAsDataURL(blob);
});
} else {
// Node.js environment
const buffer = await response.arrayBuffer();
const base64 = Buffer.from(buffer).toString('base64');
const mimeType = response.headers.get('content-type') || 'application/octet-stream';
return `data:${mimeType};base64,${base64}`;
}| export function base64ToArrayBuffer(base64: string): ArrayBuffer { | ||
| const binaryString = atob(base64); | ||
|
|
||
| const uint8Array = new Uint8Array(binaryString.length); | ||
| for (let i = 0; i < binaryString.length; i++) { | ||
| uint8Array[i] = binaryString.charCodeAt(i); | ||
| } | ||
|
|
||
| return uint8Array.buffer; | ||
| } |
There was a problem hiding this comment.
The atob function is a browser-specific API and is not available in Node.js. This will cause the code to crash when run in a Node.js environment. To ensure this library is isomorphic (runs in both browser and Node.js), you should provide a Node.js-compatible implementation.
| export function base64ToArrayBuffer(base64: string): ArrayBuffer { | |
| const binaryString = atob(base64); | |
| const uint8Array = new Uint8Array(binaryString.length); | |
| for (let i = 0; i < binaryString.length; i++) { | |
| uint8Array[i] = binaryString.charCodeAt(i); | |
| } | |
| return uint8Array.buffer; | |
| } | |
| export function base64ToArrayBuffer(base64: string): ArrayBuffer { | |
| if (typeof window !== 'undefined' && typeof window.atob === 'function') { | |
| // Browser environment | |
| const binaryString = atob(base64); | |
| const uint8Array = new Uint8Array(binaryString.length); | |
| for (let i = 0; i < binaryString.length; i++) { | |
| uint8Array[i] = binaryString.charCodeAt(i); | |
| } | |
| return uint8Array.buffer; | |
| } else { | |
| // Node.js environment | |
| return Buffer.from(base64, 'base64').buffer; | |
| } | |
| } |
| export function hexToBase64(hexstring: string): string { | ||
| const str = hexstring | ||
| .match(/\w{2}/g) | ||
| ?.map((a) => String.fromCharCode(parseInt(a, 16))) | ||
| .join(""); | ||
| return str ? btoa(str) : ""; | ||
| } |
There was a problem hiding this comment.
The btoa function is a browser-specific API and is not available in Node.js. This will cause the code to crash when run in a Node.js environment. To ensure this library is isomorphic, you should provide a Node.js-compatible implementation.
| export function hexToBase64(hexstring: string): string { | |
| const str = hexstring | |
| .match(/\w{2}/g) | |
| ?.map((a) => String.fromCharCode(parseInt(a, 16))) | |
| .join(""); | |
| return str ? btoa(str) : ""; | |
| } | |
| export function hexToBase64(hexstring: string): string { | |
| const str = hexstring | |
| .match(/\w{2}/g) | |
| ?.map((a) => String.fromCharCode(parseInt(a, 16))) | |
| .join(""); | |
| if (!str) { | |
| return ""; | |
| } | |
| if (typeof window !== 'undefined' && typeof window.btoa === 'function') { | |
| return btoa(str); | |
| } | |
| return Buffer.from(str, 'binary').toString('base64'); | |
| } |
| const response = await fetch(config.dobDecodeServerURL, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| id: 2, | ||
| jsonrpc: "2.0", | ||
| method: "dob_decode", | ||
| params: [tokenKey], | ||
| }), | ||
| }); | ||
| return response.json() as Promise<DobDecodeResponse>; |
There was a problem hiding this comment.
This function has a couple of issues that should be addressed:
- No Error Handling: The
fetchcall doesn't handle non-2xx HTTP responses. If the server returns an error (e.g., 404, 500),response.okwill befalse, but the code will proceed to callresponse.json(), which can lead to an unhandled exception. It's crucial to check the response status and handle errors appropriately. - Hardcoded JSON-RPC ID: The
idis hardcoded to2. It's a best practice to use a unique ID for each JSON-RPC request to correctly correlate requests and responses, especially in environments where multiple requests might be in-flight concurrently. UsingDate.now()or a simple counter would be a better approach.
const response = await fetch(config.dobDecodeServerURL, {
method: "POST",
headers: {
"Content-Type": "application/json",
},
body: JSON.stringify({
id: Date.now(),
jsonrpc: "2.0",
method: "dob_decode",
params: [tokenKey],
}),
});
if (!response.ok) {
const errorBody = await response.text();
throw new Error(`HTTP error! status: ${response.status}, body: ${errorBody}`);
}
return response.json() as Promise<DobDecodeResponse>;| private _queryBtcFsFn: QueryBtcFsFn = async (uri) => { | ||
| console.log("requiring", uri); | ||
| const response = await fetch( | ||
| `https://dob-decoder.ckbccc.com/restful/dob_extract_image?uri=${uri}&encode=base64`, | ||
| ); | ||
| return { | ||
| content: await response.text(), | ||
| content_type: "", | ||
| }; | ||
| }; |
There was a problem hiding this comment.
This function has a couple of issues:
- Hardcoded URL: The URL
https://dob-decoder.ckbccc.comis hardcoded, but there's a configurable_dobDecodeServerURLproperty in theConfigclass. This hardcoded URL should be replaced withthis._dobDecodeServerURLto respect the configuration. - Debug Logging:
console.logshould be removed from library code to avoid polluting the console in production environments.
private _queryBtcFsFn: QueryBtcFsFn = async (uri) => {
const response = await fetch(
`${this._dobDecodeServerURL}/restful/dob_extract_image?uri=${uri}&encode=base64`,
);
return {
content: await response.text(),
content_type: response.headers.get('content-type') || '',
};
};| "peerDependencies": { | ||
| "satori": "^0.10.13" | ||
| }, |
There was a problem hiding this comment.
Having satori in both dependencies and peerDependencies is unconventional and can lead to versioning issues. If satori is a core dependency for this package, it should only be listed in dependencies. Please remove it from peerDependencies to avoid potential conflicts for consumers of this library.
| it("should respose a decoded dob render data from a spore id", async () => { | ||
| // The spore id that you want to decode (must be a valid spore dob) | ||
| const sporeId = | ||
| "dc19e68af1793924845e2a4dbc23598ed919dcfe44d3f9cd90964fe590efb0e4"; | ||
|
|
||
| // Decode from spore id | ||
| const dob = await renderByTokenKey(sporeId); | ||
| console.log(dob); | ||
| }, 60000); |
There was a problem hiding this comment.
This test case uses console.log to manually inspect the output but lacks automated assertions to verify the correctness of the renderByTokenKey function's output. Tests should have explicit assertions to be effective for regression testing.
Additionally, the test description "should respose" contains a typo and should be "should respond".
it("should respond with decoded DOB render data from a spore id", async () => {
// The spore id that you want to decode (must be a valid spore dob)
const sporeId =
"dc19e68af1793924845e2a4dbc23598ed919dcfe44d3f9cd90964fe590efb0e4";
// Decode from spore id
const dob = await renderByTokenKey(sporeId);
expect(dob).toBeDefined();
// TODO: Add more specific assertions about the 'dob' content to make this a more effective test.
}, 60000);| export function getBackgroundColorByTraits( | ||
| traits: ParsedTrait[], | ||
| ): ParsedTrait | undefined { | ||
| return traits.find((trait) => trait.name === (Key.BgColor as string)); |
There was a problem hiding this comment.
The type cast (Key.BgColor as string) is unnecessary here. Key.BgColor is an enum member with a string value, so it can be directly compared with trait.name.
| return traits.find((trait) => trait.name === (Key.BgColor as string)); | |
| return traits.find((trait) => trait.name === Key.BgColor); |
| if (account.length > 10) { | ||
| fontSize = fontSize / 2; | ||
| } | ||
| if (account.length > 20) { | ||
| fontSize = fontSize / 2; | ||
| } | ||
| if (account.length > 30) { | ||
| fontSize = fontSize * 0.75; | ||
| } |
There was a problem hiding this comment.
The logic for adjusting fontSize uses a series of if statements without else, which causes multiple conditions to be met and applied sequentially. For example, an account name with 25 characters will have its font size halved twice. If the intention is to apply only one adjustment based on the length, you should use else if statements for clarity and correctness.
if (account.length > 30) {
fontSize = (76 / 2 / 2) * 0.75;
} else if (account.length > 20) {
fontSize = 76 / 2 / 2;
} else if (account.length > 10) {
fontSize = 76 / 2;
}| } catch (_error) { | ||
| return { | ||
| value: "", | ||
| type: "element", | ||
| name: "svg", | ||
| children: [], | ||
| attributes: {}, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The catch block currently swallows the parsing error and returns an empty SVG. While this prevents the application from crashing, it hides potentially useful error information. It would be beneficial to log the error to aid in debugging.
} catch (error) {
console.error("Error parsing SVG:", error);
return {
value: "",
type: "element",
name: "svg",
children: [],
attributes: {},
};
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the dob-render-sdk into the @ckb-ccc/spore package, introducing new rendering capabilities. The overall structure is well-organized with clear separation of concerns into parsers, renderers, and utilities. However, there are several critical issues that need attention, particularly regarding environment compatibility (browser vs. Node.js) where browser-specific APIs like atob, btoa, and FileReader are used. Additionally, there are areas for improvement in error handling, test coverage with assertions, and fixing some logical bugs in the parsing code. My review includes specific suggestions to address these points and enhance the robustness and maintainability of the new module.
| export function base64ToArrayBuffer(base64: string): ArrayBuffer { | ||
| const binaryString = atob(base64); | ||
|
|
||
| const uint8Array = new Uint8Array(binaryString.length); | ||
| for (let i = 0; i < binaryString.length; i++) { | ||
| uint8Array[i] = binaryString.charCodeAt(i); | ||
| } | ||
|
|
||
| return uint8Array.buffer; | ||
| } | ||
|
|
||
| export function isBtcFs(uri: string): uri is BtcFsURI { | ||
| return uri.startsWith("btcfs://"); | ||
| } | ||
|
|
||
| export function isIpfs(uri: string): uri is IpfsURI { | ||
| return uri.startsWith("ipfs://"); | ||
| } | ||
|
|
||
| export function hexToBase64(hexstring: string): string { | ||
| const str = hexstring | ||
| .match(/\w{2}/g) | ||
| ?.map((a) => String.fromCharCode(parseInt(a, 16))) | ||
| .join(""); | ||
| return str ? btoa(str) : ""; | ||
| } |
There was a problem hiding this comment.
The functions atob and btoa are browser-specific APIs and are not available in Node.js environments. This will cause the library to crash when used server-side. You should provide an environment-agnostic implementation, for example by using Buffer in Node.js.
| return new Promise<IpfsResult>((resolve, reject) => { | ||
| const reader = new FileReader(); | ||
|
|
||
| reader.onload = function () { | ||
| const base64 = this.result as string; | ||
| resolve(base64); | ||
| }; | ||
| reader.onerror = (error) => { | ||
| reject(new Error(`FileReader error: ${error.type}`)); | ||
| }; | ||
| reader.readAsDataURL(blob); | ||
| }); |
There was a problem hiding this comment.
The use of FileReader makes this function browser-specific and will cause it to fail in a Node.js environment. For a universal library, you should provide an environment-agnostic implementation. In Node.js, you can use Buffer.from(await blob.arrayBuffer()).toString('base64') to achieve the same result.
| parse( | ||
| styleString: string, | ||
| baseStyle?: StyleConfiguration, | ||
| ): StyleConfiguration { | ||
| try { | ||
| const input = validateString(styleString, "style string"); | ||
| const result = baseStyle ? { ...baseStyle } : { ...DEFAULT_STYLE }; | ||
|
|
||
| // Remove angle brackets if present | ||
| const cleanInput = this.removeAngleBrackets(input); | ||
|
|
||
| // Parse color | ||
| this.parseColor(cleanInput, result); | ||
|
|
||
| // Parse format | ||
| this.parseFormat(cleanInput, result); | ||
|
|
||
| // Parse alignment | ||
| this.parseAlignment(cleanInput, result); | ||
|
|
||
| // Parse break line | ||
| this.parseBreakLine(cleanInput, result); | ||
|
|
||
| return result; | ||
| } catch (error) { | ||
| throw new StyleParseError(`Failed to parse style: ${styleString}`, { | ||
| styleString, | ||
| originalError: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a logical bug in the parse method. The parseColor, parseFormat, and parseAlignment methods each return the remainder of the string after parsing, but this return value is ignored. Each function is called with the original cleanInput, meaning they don't operate sequentially on the remaining string. This should be corrected to chain the parsing operations.
parse(
styleString: string,
baseStyle?: StyleConfiguration,
): StyleConfiguration {
try {
const input = validateString(styleString, "style string");
const result = baseStyle ? { ...baseStyle } : { ...DEFAULT_STYLE };
// Remove angle brackets if present
let remaining = this.removeAngleBrackets(input);
// Parse color
remaining = this.parseColor(remaining, result);
// Parse format
remaining = this.parseFormat(remaining, result);
// Parse alignment
remaining = this.parseAlignment(remaining, result);
// Parse break line
this.parseBreakLine(remaining, result);
return result;
} catch (error) {
throw new StyleParseError(`Failed to parse style: ${styleString}`, {
styleString,
originalError: error instanceof Error ? error.message : String(error),
});
}
}| it("should respose a decoded dob render data from a spore id", async () => { | ||
| // The spore id that you want to decode (must be a valid spore dob) | ||
| const sporeId = | ||
| "dc19e68af1793924845e2a4dbc23598ed919dcfe44d3f9cd90964fe590efb0e4"; | ||
|
|
||
| // Decode from spore id | ||
| const dob = await renderByTokenKey(sporeId); | ||
| console.log(dob); | ||
| }, 60000); |
There was a problem hiding this comment.
This test is missing assertions to verify the output. It currently only checks that renderByTokenKey doesn't throw an error. Tests should include expect statements to validate the function's output against expected results. Also, there's a typo in the description ('respose' should be 'respond').
it("should respond with a decoded dob render data from a spore id", async () => {
// The spore id that you want to decode (must be a valid spore dob)
const sporeId =
"dc19e68af1793924845e2a4dbc23598ed919dcfe44d3f9cd90964fe590efb0e4";
// Decode from spore id
const dob = await renderByTokenKey(sporeId);
console.log(dob);
// TODO: Add assertions to validate the `dob` content.
// For example: expect(dob).toContain('<svg');
}, 60000);| ) { | ||
| let renderData: RenderOutput; | ||
| if (typeof renderOutput === "string") { | ||
| renderData = JSON.parse(renderOutput) as RenderOutput; |
There was a problem hiding this comment.
The type assertion as RenderOutput is unsafe as it doesn't guarantee the parsed JSON matches the RenderOutput type. This could lead to runtime errors if the input string is malformed or has an unexpected structure. Consider using a validation library (like Zod) or a type guard function to safely parse and validate the data.
| throw new Error( | ||
| `Failed to render text: ${error instanceof Error ? error.message : String(error)}`, | ||
| ); |
There was a problem hiding this comment.
Instead of throwing a generic Error, it's better to use a custom error class like RenderEngineError from types/errors.ts. This provides more specific error information to the caller.
| private shouldAppendToPrevious( | ||
| _element: RenderElement, | ||
| _elements: RenderElement[], | ||
| ): boolean { | ||
| // This would need to be implemented based on the original logic | ||
| // For now, returning false to maintain current behavior | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Appends element to the previous element | ||
| */ | ||
| private appendToPreviousElement( | ||
| element: RenderElement, | ||
| elements: RenderElement[], | ||
| ): void { | ||
| const lastElement = elements[elements.length - 1]; | ||
| if (!lastElement) return; | ||
|
|
||
| element.type = "span"; | ||
| delete element.props.style.width; | ||
| element.props.style.display = "block"; | ||
|
|
||
| if (Array.isArray(lastElement.props.children)) { | ||
| lastElement.props.children.push(element); | ||
| } else { | ||
| lastElement.props.children = [lastElement.props.children, element]; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Adds break lines for an item | ||
| */ | ||
| private addBreakLines(item: TextItem, elements: RenderElement[]): void { | ||
| for (let i = 1; i < item.parsedStyle.breakLine; i++) { | ||
| elements.push({ | ||
| key: `${item.name}${i}`, | ||
| type: "p", | ||
| props: { | ||
| children: "", | ||
| style: { | ||
| height: "36px", | ||
| margin: 0, | ||
| }, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
There are a couple of areas for improvement here:
- The
shouldAppendToPreviousmethod always returnsfalse, which makes theappendToPreviousElementmethod dead code. This seems to be an incomplete feature. - The
addBreakLinesmethod uses a hardcoded height of'36px'. This magic number should be defined as a constant inRENDER_CONSTANTSfor better maintainability.
| return "image/avif"; | ||
| } | ||
|
|
||
| console.log("Unsupported MIME type", base64Header); |
There was a problem hiding this comment.
Using console.log for unsupported MIME types in a library is not ideal as it can add noise to the consumer's application logs. Consider using a configurable logger or removing it if it's only for debugging.
| console.log("dob-render-sdk requiring", uri); | ||
| const response = await fetch( | ||
| `https://dob-decoder.ckbccc.com/restful/dob_extract_image?uri=${uri}&encode=base64`, | ||
| ); | ||
| const text = await response.text(); | ||
| return { | ||
| content: text, | ||
| content_type: "", | ||
| }; | ||
| }; | ||
|
|
||
| private _queryUrlFn = async (url: string) => { | ||
| console.log("dob-render-sdk requiring", url); |
There was a problem hiding this comment.
The use of console.log in library code is generally discouraged as it can pollute the console of the consuming application. It's better to use a dedicated, configurable logger for debugging purposes.
| props?: Pick<RenderProps, "font"> & { | ||
| outputType?: "svg"; | ||
| }, |
There was a problem hiding this comment.
The outputType property within the props object is defined but not used anywhere in the function. This appears to be dead code or an incomplete feature. It should be removed to avoid confusion, or if intended for future use, a // TODO comment should be added to clarify its purpose.
|
/canary |
|
🚀 Canary version published successfully! View workflow run The following packages have been published to npm:
|
2 participants
Migration of legacy dob-render-sdk
Consider to continuously maintain dob-render-sdk, we aim to migrate the essential part of it directly into our Spore module, with minimal adaptation about replacing the connected backend server with our official one, which provides both
fsuri-parseservice andDob decodeservice.