feat: Implement SSR rendering pipeline #104
Conversation
Summary of ChangesHello @yrom, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the AntV Infographic library by introducing Server-Side Rendering (SSR) capabilities. This allows for the generation of SVG infographics in a Node.js environment, eliminating the need for a browser. A new command-line interface (CLI) tool is also provided, simplifying the process of converting infographic syntax into SVG files. The changes include robust DOM emulation using Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a significant new feature: Server-Side Rendering (SSR) capabilities and a command-line interface (CLI) for rendering infographics. The changes are extensive, including a new cli package, DOM mocking with jsdom, and adjustments to the rendering pipeline to support a non-browser environment. The implementation is generally solid, with good use of jsdom for environment setup and a well-structured CLI.
I've identified a critical issue in the new SSR tests where asynchronous functions are not being awaited, which will cause tests to pass incorrectly. I've also found several medium-severity issues, including dead code, unnecessary package contents, and opportunities to improve type safety and test quality. Addressing these points will improve the robustness and maintainability of this new functionality.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #104 +/- ##
==========================================
+ Coverage 31.37% 35.71% +4.33%
==========================================
Files 314 317 +3
Lines 23149 23454 +305
Branches 1257 1495 +238
==========================================
+ Hits 7264 8377 +1113
+ Misses 15882 15073 -809
- Partials 3 4 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@yrom Thanks for your contribution. We’ll review this PR asap on workdays. |
|
@yrom Before initiating the human code review, it is recommended to first resolve the issues raised by Gemini. |
Aarebecca
force-pushed
the
feature/ssr
branch
from
832bedd to
31c48e6
Compare
| @@ -0,0 +1,11 @@ | |||
| infographic list-row-simple-horizontal-arrow | |||
There was a problem hiding this comment.
把这些作为 infographic 的单元测试的输入，测试 ssr 的 api，然后把输出的 svg 也存到 git commit 中，单测断言做 svg string 的匹配。
There was a problem hiding this comment.
If do that, the unit tests will be unstable, since it affected by rendering logic changes.
| @@ -0,0 +1,70 @@ | |||
| #!/usr/bin/env node | |||
|
@copilot Code Review |
|
@yrom 挺牛的 PR，增加了两个大能力，可以在 PR 描述中加上描述：
这两个功能，后续都需要在官网文档、Readme 中展示出来。 |
Aarebecca
changed the title
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: Server-Side Rendering (SSR) for infographics. This is achieved by adding a new ssr module that uses linkedom to create a mock DOM environment in Node.js. A key part of this is a sophisticated DOM shim that correctly handles SVG serialization and text measurement on the server. The changes also include a new @antv/infographic-cli package for command-line rendering, complete with its own tests and documentation. The core library is updated to support the SSR mode, for instance by skipping font loading on the server and using textContent instead of innerText. Overall, this is a well-structured and comprehensive implementation of SSR support. My review includes a few minor suggestions for improving code clarity and maintainability.
| const factor = parseFloat(lineHeightStyle); | ||
| lineHeight = isNaN(factor) ? fontSize * 1.4 : (factor < 4 ? fontSize * factor : factor); | ||
| } |
There was a problem hiding this comment.
This logic for calculating lineHeight uses magic numbers (1.4 and 4). It appears to differentiate between a unitless multiplier and an absolute pixel value. To improve readability and maintainability, please add a comment explaining this heuristic.
| export interface SSRRenderOptions { | ||
| /** Input: Antv Infographic Syntax string */ | ||
| input: string; | ||
| options?: Partial<InfographicOptions>; | ||
| } |
There was a problem hiding this comment.
这个 renderToSVG API 参数可以好好看看 @Aarebecca
- 参数名称合理不
- 后面再扩展支持加哪里，比如为了更加的精度，node 环境用 canvas 测评文本宽度支持，怎么传下去
| @@ -0,0 +1,3 @@ | |||
| // Re-export SSR functionality from main package | |||
| export { renderToSVG, setupDOM, teardownDOM, isSSR } from '@antv/infographic/ssr'; | |||
| export type { SSRRenderOptions, SSRRenderResult } from '@antv/infographic/ssr'; | |||
There was a problem hiding this comment.
抛一下我的想法吧，保持架构上和合理性。
- @antv/infographic 代码可以在浏览器环境和 node 环境跑起来（出 svg 的代码），核心代码兼容，可两端跑。
- @antv/infographic 里不应该依赖 jsdom 和 linkdom svgo 这些，这些是为了服务端场景用的。
- cli 就是为 node 环境用的，定位不只是 cli，更像是 @antv/infographic-srr，里面可以包node环境的布丁（linkdom，svgo等）。
- 用户可以用 @antv/infographic 封装等轻量 renderSVG API，在浏览器环境直接可运行。如果在 node 环境用这个，需要主动给 global 加 dom 环境变量。
- 用户也可以用我们在 node 环境封装好的 cli 这个包，直接就可以用。
建议 cli 这个包改名字，叫 @antv/infographic-srr，提供以下能力
- 有 cli 命令可用
- 用户可直接导入 renderSvg 方法，在 node 环境直接可以用跑起来
- 可提供 svg 压缩，可以提供渲染成图片等，
- 也可以提供一些语法糖 exportToFile、toBuffer，这样在服务端场景更加实用一些，才很贴合
- 风格，可以参考 @antv/gpt-vis-ssr、@antv/g6-ssr
There was a problem hiding this comment.
我觉得 G2、G6 单独一个 ssr 包不是因为 G2、G6 不支持 ssr，而是用起来麻烦。
remove custom XMLSerializer implementation in favor of native outerHTML move svgo optimization to CLI level with option
- Add fast fail for syntax errors - Validate template existence
|
这个 PR 的设计还存在诸多存在争议的点，为了避免后续对用户造成疑惑，所以我们需要花费更多的时间进行讨论和明确设计 |
|
To keep things manageable, I intentionally started simple: SSR just works out of the box, backed by a minimal CLI. IMHO, we can gradually add more advanced features later as needed. |
|
cli 其实是 ssr 能力的一种透出形态。 我觉得可以先提供 ssr 的 api，再来做 cli 相关的，而且我更建议 cli 作为项目周边，不放在这个仓库中，可以放到个人仓库，然后官网收录，因为这类封装可以有更多的形态。 然后 ssr 的 api 有多种设计方式。
我其实更建议第一种，可以类似于 ReactDOM，这个可以讨论下。@Aarebecca @yrom |
|
对于单测，现在 cli 中的单测应该放到 infographic 主包中，测试 ssr 的能力。 cli 测试可以简化一下。 |
|
@yrom 有啥进展不，要不加一个微信聊一下，我的微信在我 GitHub 主页有。 |
|
@hustcc 🈚️ 我没明白你们具体改进方向，另外这几天也没时间😂 |
|
@yrom 我把 PR 先合并到开发分支了，后续我会进行处理 |
5 participants
Add SSR mode support (global flag
__ANTV_INFOGRAPHIC_SSR__).LinkeDOMfor mocking DOM.Test:
Related #68
Non-Browser Rendering
With custom ResourceLoader:
CLI Package documents
cli/README.md
npx @antv/infographic-cli input.txt -o output.svg( available on@antv/infographic-clipublished)