feat: + gadget-Shiki #579
Open
feat: + gadget-Shiki #579
+195
−1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gui-ying233
suggested changes
Dec 5, 2024
Comment on lines
+16
to
+20
| const shiki = await import(SHIKI_CDN); | ||
| TARGET_ELEMENTS.forEach((el) => { | ||
| renderBlock(shiki, el); | ||
| }); | ||
| mw.hook("npm:shiki").fire(shiki); |
There was a problem hiding this comment.
VM20415:59 Uncaught (in promise) ReferenceError: Cannot access 'getLangFromElement' before initialization
这5行是不是应该放在最后（？
| } | ||
| return [langInfo.aliases?.[0], langInfo.name, langInfo.id, lang].filter(Boolean).sort((a, b) => a.length - b.length)[0]; | ||
| })(); | ||
| console.info("[SHIKI]", "Rendering", el, lang, langInfo); |
There was a problem hiding this comment.
能不能不要拉屎（至少改成console.debug吧（
Suggested change
| console.info("[SHIKI]", "Rendering", el, lang, langInfo); | |
| console.debug("[SHIKI]", "Rendering", el, lang, langInfo); |
Comment on lines
+27
to
+28
| counter-reset: step; | ||
| counter-increment: step calc(var(--start, 1) - 1); |
There was a problem hiding this comment.
为啥不直接
Suggested change
| counter-reset: step; | |
| counter-increment: step calc(var(--start, 1) - 1); | |
| counter-reset: step calc(var(--start, 1) - 1); |
gui-ying233
suggested changes
Dec 5, 2024
Comment on lines
+106
to
+111
| el.style.display = "none"; | ||
| el.dataset.shikiRendered = "1"; | ||
| const wrapper = document.createElement("div"); | ||
| wrapper.innerHTML = html; | ||
| const pre = wrapper.querySelector("pre"); | ||
| el.insertAdjacentElement("afterend", pre); |
gui-ying233
reviewed
Dec 5, 2024
| { | ||
| pre: (node) => { | ||
| node.properties.class += ` lang-${langLabel}`; | ||
| node.properties.style += ";"; |
|
@sourcery-ai review |
审阅者指南 by Sourcery这个拉取请求引入了一个名为"Shiki"的新小工具，用于代码高亮。它添加了对 Shiki 高亮器的支持，作为现有 Prism 高亮器的替代方案。实现涉及添加新的小工具定义、JavaScript 文件、CSS 文件，并更新现有的 code-prettify 小工具以检查"gadget-Shiki"用户选项。 Shiki 代码高亮过程的序列图sequenceDiagram
participant User
participant Page
participant ShikiGadget
participant ShikiCDN
User->>Page: 查看包含代码块的页面
activate Page
Page->>ShikiGadget: 初始化
activate ShikiGadget
ShikiGadget->>ShikiCDN: 从 CDN 导入 Shiki
ShikiCDN-->>ShikiGadget: 返回 Shiki 库
ShikiGadget->>ShikiGadget: 查找目标元素
loop 对每个代码块
ShikiGadget->>ShikiGadget: 获取元素的语言
ShikiGadget->>ShikiGadget: 渲染代码块
ShikiGadget->>Page: 插入高亮代码
end
ShikiGadget-->>Page: 触发 'npm:shiki' 钩子
deactivate ShikiGadget
Page-->>User: 显示高亮代码
deactivate Page
Shiki 小工具结构的类图classDiagram
class ShikiGadget {
+SHIKI_CDN: string
+TARGET_ELEMENTS: NodeList
+getLangFromContentModel()
+getLangFromElement(el)
+renderBlock(shiki, el)
}
class ShikiStyles {
+pre.shiki
+shiki-lang-badge
+line-number
}
class GadgetDefinition {
+ResourceLoader: boolean
+hidden: boolean
+default: boolean
+type: string
+dependencies: array
}
ShikiGadget --> ShikiStyles: 使用
GadgetDefinition --> ShikiGadget: 定义
代码块渲染过程的状态图stateDiagram-v2
[*] --> CheckGadget
CheckGadget --> Exit: 如果启用了 Prism 或 Shiki
CheckGadget --> FindElements: 否则
FindElements --> GetLanguage
GetLanguage --> FromElement: 检查元素属性
GetLanguage --> FromContentModel: 检查页面属性
FromElement --> RenderBlock
FromContentModel --> RenderBlock
RenderBlock --> ApplyStyles
ApplyStyles --> DisplayResult
DisplayResult --> [*]
文件级变更
提示和命令与 Sourcery 交互
自定义您的体验访问您的仪表板以：
获取帮助Original review guide in EnglishReviewer's Guide by SourceryThis pull request introduces a new gadget named "Shiki" for code highlighting. It adds support for the Shiki highlighter as an alternative to the existing Prism highlighter. The implementation involves adding a new gadget definition, JavaScript file, CSS file, and updating the existing code-prettify gadget to check for the "gadget-Shiki" user option. Sequence diagram for Shiki code highlighting processsequenceDiagram
participant User
participant Page
participant ShikiGadget
participant ShikiCDN
User->>Page: Views page with code blocks
activate Page
Page->>ShikiGadget: Initialize
activate ShikiGadget
ShikiGadget->>ShikiCDN: Import Shiki from CDN
ShikiCDN-->>ShikiGadget: Return Shiki library
ShikiGadget->>ShikiGadget: Find target elements
loop For each code block
ShikiGadget->>ShikiGadget: Get language from element
ShikiGadget->>ShikiGadget: Render code block
ShikiGadget->>Page: Insert highlighted code
end
ShikiGadget-->>Page: Fire 'npm:shiki' hook
deactivate ShikiGadget
Page-->>User: Display highlighted code
deactivate Page
Class diagram for Shiki gadget structureclassDiagram
class ShikiGadget {
+SHIKI_CDN: string
+TARGET_ELEMENTS: NodeList
+getLangFromContentModel()
+getLangFromElement(el)
+renderBlock(shiki, el)
}
class ShikiStyles {
+pre.shiki
+shiki-lang-badge
+line-number
}
class GadgetDefinition {
+ResourceLoader: boolean
+hidden: boolean
+default: boolean
+type: string
+dependencies: array
}
ShikiGadget --> ShikiStyles: uses
GadgetDefinition --> ShikiGadget: defines
State diagram for code block rendering processstateDiagram-v2
[*] --> CheckGadget
CheckGadget --> Exit: if Prism or Shiki enabled
CheckGadget --> FindElements: else
FindElements --> GetLanguage
GetLanguage --> FromElement: Check element attributes
GetLanguage --> FromContentModel: Check page properties
FromElement --> RenderBlock
FromContentModel --> RenderBlock
RenderBlock --> ApplyStyles
ApplyStyles --> DisplayResult
DisplayResult --> [*]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
嘿 @dragon-fish - 我已经审查了你的更改 - 这里有一些反馈：
总体评论：
- 考虑将 Shiki 本地打包，而不是从 esm.sh CDN 加载，以提高安全性和可靠性
这是我在审查期间查看的内容
- 🟡 一般问题：发现 2 个问题
- 🟢 安全性：一切看起来都很好
- 🟢 测试：一切看起来都很好
- 🟡 复杂性：发现 1 个问题
- 🟢 文档：一切看起来都很好
帮助我变得更有用！请点击每条评论上的 👍 或 👎，我将使用反馈来改进你的评论。
Original comment in English
Hey @dragon-fish - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider bundling Shiki locally instead of loading from esm.sh CDN to improve security and reliability
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| "use strict"; | ||
|
|
||
| (async () => { | ||
| const SHIKI_CDN = "https://esm.sh/shiki@1.24.0"; |
There was a problem hiding this comment.
建议： 考虑为 Shiki 导入添加备用 CDN 和超时处理
外部 CDN 依赖可能会失败。考虑实现备用 CDN 并添加超时处理，以在 CDN 不可用时优雅降级。
Original comment in English
suggestion: Consider adding fallback CDN and timeout handling for the Shiki import
External CDN dependencies can fail. Consider implementing a fallback CDN and adding timeout handling to gracefully degrade when the CDN is unavailable.
| TARGET_ELEMENTS.forEach((el) => { | ||
| renderBlock(shiki, el); | ||
| }); | ||
| mw.hook("npm:shiki").fire(shiki); |
There was a problem hiding this comment.
问题 (bug_risk)： 钩子在所有块渲染之前触发，这可能导致竞争条件
考虑在所有块渲染后触发钩子，方法是等待所有 renderBlock 承诺解决。
Original comment in English
issue (bug_risk): Hook is fired before all blocks are rendered, which could cause race conditions
Consider firing the hook after all blocks have been rendered by waiting for all renderBlock promises to resolve.
|
|
||
| "use strict"; | ||
|
|
||
| (async () => { |
There was a problem hiding this comment.
问题 (复杂性)： 考虑通过使用 async/await 并将转换器配置提取到单独的变量中来简化 renderBlock 函数。
renderBlock 函数可以在保持功能的同时简化：
- 使用 async/await 而不是 promise 链
- 提取转换器配置以减少嵌套
这是一个建议的重构：
Original comment in English
issue (complexity): Consider simplifying the renderBlock function by using async/await and extracting the transformer configuration into a separate variable.
The renderBlock function could be simplified while maintaining functionality:
- Use async/await instead of promise chains
- Extract transformer config to reduce nesting
Here's a suggested refactor:
const shikiTransformer = {
pre: (node) => {
node.properties.class += ` lang-${langLabel}`;
node.properties.style += ";";
node.properties.style += `padding-right: ${(10 * langLabel.length + 12).toFixed()}px;`;
if (lineFrom) {
node.properties.class += " line-number";
}
},
code: (node) => {
node.properties.class += " shiki-code";
node.tagName = "div";
node.properties.style += `;--start: ${+lineFrom};`;
},
line: (node, line) => {
node.properties["data-line-number-raw"] = line;
},
postprocess: (html) => {
if (langLabel) {
return html.replace("</pre>", `<span class="shiki-lang-badge">${langLabel}</span></pre>`);
}
},
};
async function renderBlock(shiki, el) {
if (el.classList.contains("shiki") || !!el.dataset.shikiRendered) {
return null;
}
const lang = getLangFromElement(el) || getLangFromContentModel();
if (!lang) {
return null;
}
try {
const html = await shiki.codeToHtml(el.innerText.trimEnd(), {
lang,
theme: "one-dark-pro",
transformers: [shikiTransformer],
});
el.style.display = "none";
el.dataset.shikiRendered = "1";
const wrapper = document.createElement("div");
wrapper.innerHTML = html;
const pre = wrapper.querySelector("pre");
el.insertAdjacentElement("afterend", pre);
return pre;
} catch (e) {
console.error("[SHIKI] Render failed", el, e);
return null;
}
}
</details>
Comment on lines
+55
to
+119
| async function renderBlock(shiki, el) { | ||
| if (el.classList.contains("shiki") || !!el.dataset.shikiRendered) { | ||
| return Promise.resolve(null); | ||
| } | ||
| const lang = getLangFromElement(el) || getLangFromContentModel(); | ||
| if (!lang) { | ||
| return Promise.resolve(null); | ||
| } | ||
|
|
||
| const langInfo = shiki.bundledLanguagesInfo.find((i) => i.aliases?.includes(lang) || i.id === lang || i.name === lang); | ||
| const langLabel = (() => { | ||
| if (!langInfo) { | ||
| return lang; | ||
| } | ||
| return [langInfo.aliases?.[0], langInfo.name, langInfo.id, lang].filter(Boolean).sort((a, b) => a.length - b.length)[0]; | ||
| })(); | ||
| console.info("[SHIKI]", "Rendering", el, lang, langInfo); | ||
|
|
||
| const lineFrom = el.dataset.lineFrom || el.dataset.from || "1"; | ||
|
|
||
| const renderedEl = await shiki | ||
| .codeToHtml(el.innerText.trimEnd(), { | ||
| lang, | ||
| theme: "one-dark-pro", | ||
| transformers: [ | ||
| { | ||
| pre: (node) => { | ||
| node.properties.class += ` lang-${langLabel}`; | ||
| node.properties.style += ";"; | ||
| node.properties.style += `padding-right: ${(10 * langLabel.length + 12).toFixed()}px;`; | ||
| if (lineFrom) { | ||
| node.properties.class += " line-number"; | ||
| } | ||
| }, | ||
| code: (node) => { | ||
| node.properties.class += " shiki-code"; | ||
| node.tagName = "div"; | ||
| node.properties.style += `;--start: ${+lineFrom};`; | ||
| }, | ||
| line: (node, line) => { | ||
| node.properties["data-line-number-raw"] = line; | ||
| }, | ||
| postprocess: (html) => { | ||
| if (langLabel) { | ||
| return html.replace("</pre>", `<span class="shiki-lang-badge">${langLabel}</span></pre>`); | ||
| } | ||
| }, | ||
| }, | ||
| ], | ||
| }) | ||
| .then((html) => { | ||
| el.style.display = "none"; | ||
| el.dataset.shikiRendered = "1"; | ||
| const wrapper = document.createElement("div"); | ||
| wrapper.innerHTML = html; | ||
| const pre = wrapper.querySelector("pre"); | ||
| el.insertAdjacentElement("afterend", pre); | ||
| return pre; | ||
| }) | ||
| .catch((e) => { | ||
| console.error("[SHIKI] Render failed", el, e); | ||
| return null; | ||
| }); | ||
| return renderedEl; | ||
| }; |
There was a problem hiding this comment.
问题 (代码质量)： 避免在块内声明函数，倾向于使用函数赋值表达式。 (avoid-function-declarations-in-blocks)
解释
函数声明可能在 Javascript 中被提升，但不同浏览器之间的行为不一致。 提升通常令人困惑，应避免。在块内使用函数声明时， 应使用函数表达式，这会在作用域内创建函数。Original comment in English
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.
Comment on lines
+75
to
+118
| const renderedEl = await shiki | ||
| .codeToHtml(el.innerText.trimEnd(), { | ||
| lang, | ||
| theme: "one-dark-pro", | ||
| transformers: [ | ||
| { | ||
| pre: (node) => { | ||
| node.properties.class += ` lang-${langLabel}`; | ||
| node.properties.style += ";"; | ||
| node.properties.style += `padding-right: ${(10 * langLabel.length + 12).toFixed()}px;`; | ||
| if (lineFrom) { | ||
| node.properties.class += " line-number"; | ||
| } | ||
| }, | ||
| code: (node) => { | ||
| node.properties.class += " shiki-code"; | ||
| node.tagName = "div"; | ||
| node.properties.style += `;--start: ${+lineFrom};`; | ||
| }, | ||
| line: (node, line) => { | ||
| node.properties["data-line-number-raw"] = line; | ||
| }, | ||
| postprocess: (html) => { | ||
| if (langLabel) { | ||
| return html.replace("</pre>", `<span class="shiki-lang-badge">${langLabel}</span></pre>`); | ||
| } | ||
| }, | ||
| }, | ||
| ], | ||
| }) | ||
| .then((html) => { | ||
| el.style.display = "none"; | ||
| el.dataset.shikiRendered = "1"; | ||
| const wrapper = document.createElement("div"); | ||
| wrapper.innerHTML = html; | ||
| const pre = wrapper.querySelector("pre"); | ||
| el.insertAdjacentElement("afterend", pre); | ||
| return pre; | ||
| }) | ||
| .catch((e) => { | ||
| console.error("[SHIKI] Render failed", el, e); | ||
| return null; | ||
| }); | ||
| return renderedEl; |
There was a problem hiding this comment.
建议 (代码质量)： 内联立即返回的变量 (inline-immediately-returned-variable)
Suggested change
| const renderedEl = await shiki | |
| .codeToHtml(el.innerText.trimEnd(), { | |
| lang, | |
| theme: "one-dark-pro", | |
| transformers: [ | |
| { | |
| pre: (node) => { | |
| node.properties.class += ` lang-${langLabel}`; | |
| node.properties.style += ";"; | |
| node.properties.style += `padding-right: ${(10 * langLabel.length + 12).toFixed()}px;`; | |
| if (lineFrom) { | |
| node.properties.class += " line-number"; | |
| } | |
| }, | |
| code: (node) => { | |
| node.properties.class += " shiki-code"; | |
| node.tagName = "div"; | |
| node.properties.style += `;--start: ${+lineFrom};`; | |
| }, | |
| line: (node, line) => { | |
| node.properties["data-line-number-raw"] = line; | |
| }, | |
| postprocess: (html) => { | |
| if (langLabel) { | |
| return html.replace("</pre>", `<span class="shiki-lang-badge">${langLabel}</span></pre>`); | |
| } | |
| }, | |
| }, | |
| ], | |
| }) | |
| .then((html) => { | |
| el.style.display = "none"; | |
| el.dataset.shikiRendered = "1"; | |
| const wrapper = document.createElement("div"); | |
| wrapper.innerHTML = html; | |
| const pre = wrapper.querySelector("pre"); | |
| el.insertAdjacentElement("afterend", pre); | |
| return pre; | |
| }) | |
| .catch((e) => { | |
| console.error("[SHIKI] Render failed", el, e); | |
| return null; | |
| }); | |
| return renderedEl; | |
| return await shiki | |
| .codeToHtml(el.innerText.trimEnd(), { | |
| lang, | |
| theme: "one-dark-pro", | |
| transformers: [ | |
| { | |
| pre: (node) => { | |
| node.properties.class += ` lang-${langLabel}`; | |
| node.properties.style += ";"; | |
| node.properties.style += `padding-right: ${(10 * langLabel.length + 12).toFixed()}px;`; | |
| if (lineFrom) { | |
| node.properties.class += " line-number"; | |
| } | |
| }, | |
| code: (node) => { | |
| node.properties.class += " shiki-code"; | |
| node.tagName = "div"; | |
| node.properties.style += `;--start: ${+lineFrom};`; | |
| }, | |
| line: (node, line) => { | |
| node.properties["data-line-number-raw"] = line; | |
| }, | |
| postprocess: (html) => { | |
| if (langLabel) { | |
| return html.replace("</pre>", `<span class="shiki-lang-badge">${langLabel}</span></pre>`); | |
| } | |
| }, | |
| }, | |
| ], | |
| }) | |
| .then((html) => { | |
| el.style.display = "none"; | |
| el.dataset.shikiRendered = "1"; | |
| const wrapper = document.createElement("div"); | |
| wrapper.innerHTML = html; | |
| const pre = wrapper.querySelector("pre"); | |
| el.insertAdjacentElement("afterend", pre); | |
| return pre; | |
| }) | |
| .catch((e) => { | |
| console.error("[SHIKI] Render failed", el, e); | |
| return null; | |
| }); | |
解释
我们经常在代码中看到的是将结果赋值给一个变量然后立即返回它。
直接返回结果可以缩短代码并删除不必要的
变量，减少阅读函数的心理负担。
中间变量可能有用的地方是如果它们随后被用作
参数或条件，并且名称可以作为变量代表的注释。在你从函数返回它的情况下，
函数名称用于告诉你结果是什么，因此变量名称
是不必要的。
Original comment in English
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
Suggested change
| const renderedEl = await shiki | |
| .codeToHtml(el.innerText.trimEnd(), { | |
| lang, | |
| theme: "one-dark-pro", | |
| transformers: [ | |
| { | |
| pre: (node) => { | |
| node.properties.class += ` lang-${langLabel}`; | |
| node.properties.style += ";"; | |
| node.properties.style += `padding-right: ${(10 * langLabel.length + 12).toFixed()}px;`; | |
| if (lineFrom) { | |
| node.properties.class += " line-number"; | |
| } | |
| }, | |
| code: (node) => { | |
| node.properties.class += " shiki-code"; | |
| node.tagName = "div"; | |
| node.properties.style += `;--start: ${+lineFrom};`; | |
| }, | |
| line: (node, line) => { | |
| node.properties["data-line-number-raw"] = line; | |
| }, | |
| postprocess: (html) => { | |
| if (langLabel) { | |
| return html.replace("</pre>", `<span class="shiki-lang-badge">${langLabel}</span></pre>`); | |
| } | |
| }, | |
| }, | |
| ], | |
| }) | |
| .then((html) => { | |
| el.style.display = "none"; | |
| el.dataset.shikiRendered = "1"; | |
| const wrapper = document.createElement("div"); | |
| wrapper.innerHTML = html; | |
| const pre = wrapper.querySelector("pre"); | |
| el.insertAdjacentElement("afterend", pre); | |
| return pre; | |
| }) | |
| .catch((e) => { | |
| console.error("[SHIKI] Render failed", el, e); | |
| return null; | |
| }); | |
| return renderedEl; | |
| return await shiki | |
| .codeToHtml(el.innerText.trimEnd(), { | |
| lang, | |
| theme: "one-dark-pro", | |
| transformers: [ | |
| { | |
| pre: (node) => { | |
| node.properties.class += ` lang-${langLabel}`; | |
| node.properties.style += ";"; | |
| node.properties.style += `padding-right: ${(10 * langLabel.length + 12).toFixed()}px;`; | |
| if (lineFrom) { | |
| node.properties.class += " line-number"; | |
| } | |
| }, | |
| code: (node) => { | |
| node.properties.class += " shiki-code"; | |
| node.tagName = "div"; | |
| node.properties.style += `;--start: ${+lineFrom};`; | |
| }, | |
| line: (node, line) => { | |
| node.properties["data-line-number-raw"] = line; | |
| }, | |
| postprocess: (html) => { | |
| if (langLabel) { | |
| return html.replace("</pre>", `<span class="shiki-lang-badge">${langLabel}</span></pre>`); | |
| } | |
| }, | |
| }, | |
| ], | |
| }) | |
| .then((html) => { | |
| el.style.display = "none"; | |
| el.dataset.shikiRendered = "1"; | |
| const wrapper = document.createElement("div"); | |
| wrapper.innerHTML = html; | |
| const pre = wrapper.querySelector("pre"); | |
| el.insertAdjacentElement("afterend", pre); | |
| return pre; | |
| }) | |
| .catch((e) => { | |
| console.error("[SHIKI] Render failed", el, e); | |
| return null; | |
| }); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
AnnAngela
requested changes
Jan 1, 2025
There was a problem hiding this comment.
✖ ./src/gadgets/Shiki/definition.yaml is invalid
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
添加 Shiki 代码高亮器作为一个小工具。
新特性：
测试：
Original summary in English
Summary by Sourcery
Add Shiki code highlighter as a gadget.
New Features:
Tests: