Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(transformers): Ignore empty lines on notation transformer range #755

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
41 changes: 21 additions & 20 deletions packages/transformers/src/transformers/notation-highlight-word.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { ShikiTransformer } from 'shiki'
import { highlightWordInLine } from '../shared/highlight-word'
import { createCommentNotationTransformer } from '../utils'
import { createCommentNotationTransformerExperimental } from '../utils'

export interface TransformerNotationWordHighlightOptions {
/**
Expand All @@ -13,33 +13,34 @@ export interface TransformerNotationWordHighlightOptions {
classActivePre?: string
}

const regex = /\[!code word:((?:\\.|[^:\]])+)(:\d+)?\]/g

export function transformerNotationWordHighlight(
options: TransformerNotationWordHighlightOptions = {},
): ShikiTransformer {
const {
classActiveWord = 'highlighted-word',
classActivePre = undefined,
classActivePre,
} = options

return createCommentNotationTransformer(
return createCommentNotationTransformerExperimental(
'@shikijs/transformers:notation-highlight-word',
// comment-start | marker | word | range | comment-end
/^\s*(?:\/\/|\/\*|<!--|#)\s+\[!code word:((?:\\.|[^:\]])+)(:\d+)?\]\s*(?:\*\/|-->)?/,
function ([_, word, range], _line, comment, lines, index) {
const lineNum = range ? Number.parseInt(range.slice(1), 10) : lines.length

// escape backslashes
word = word.replace(/\\(.)/g, '$1')

lines
// Don't include the comment itself
.slice(index + 1, index + 1 + lineNum)
.forEach(line => highlightWordInLine.call(this, line, comment, word, classActiveWord))

if (classActivePre)
this.addClassToHast(this.pre, classActivePre)
return true
function (text, _line, comment, lines, index) {
return text.replace(regex, (_, word, range) => {
const lineNum = range ? Number.parseInt(range.slice(1), 10) : lines.length

// escape backslashes
word = word.replace(/\\(.)/g, '$1')

lines
.slice(index, index + lineNum)
.forEach(line => highlightWordInLine.call(this, line, comment, word, classActiveWord))

if (classActivePre)
this.addClassToHast(this.pre, classActivePre)

return ''
})
},
true, // remove empty lines
)
}
32 changes: 19 additions & 13 deletions packages/transformers/src/transformers/notation-map.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { ShikiTransformer } from 'shiki'
import { createCommentNotationTransformer } from '../utils'
import { createCommentNotationTransformerExperimental } from '../utils'

export interface TransformerNotationMapOptions {
classMap?: Record<string, string | string[]>
Expand All @@ -22,19 +22,25 @@ export function transformerNotationMap(
classActivePre = undefined,
} = options

return createCommentNotationTransformer(
const regex = new RegExp(`\\[!code (${Object.keys(classMap).map(escapeRegExp).join('|')})(:\\d+)?\\]`, 'g')

return createCommentNotationTransformerExperimental(
name,
new RegExp(`\\s*(?://|/\\*|<!--|#|--|%{1,2}|;{1,2}|"|')\\s+\\[!code (${Object.keys(classMap).map(escapeRegExp).join('|')})(:\\d+)?\\]\\s*(?:\\*/|-->)?\\s*$`),
function ([_, match, range = ':1'], _line, _comment, lines, index) {
const lineNum = Number.parseInt(range.slice(1), 10)
lines
.slice(index, index + lineNum)
.forEach((line) => {
this.addClassToHast(line, classMap[match])
})
if (classActivePre)
this.addClassToHast(this.pre, classActivePre)
return true
function (text, _line, _comment, lines, index) {
return text.replace(regex, (_, group, range = ':1') => {
const lineNum = Number.parseInt(range.slice(1), 10)

lines
.slice(index, index + lineNum)
.forEach((line) => {
this.addClassToHast(line, classMap[group])
})

if (classActivePre)
this.addClassToHast(this.pre, classActivePre)

return ''
})
},
)
}
63 changes: 63 additions & 0 deletions packages/transformers/src/utils-legacy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import type { Element, Text } from 'hast'
import type { ShikiTransformer, ShikiTransformerContext } from 'shiki'

export function createCommentNotationTransformer(
name: string,
regex: RegExp,
onMatch: (
this: ShikiTransformerContext,
match: string[],
line: Element,
commentNode: Element,
lines: Element[],
index: number,
) => boolean,
removeEmptyLines = false,
): ShikiTransformer {
return {
name,
code(code) {
const lines = code.children.filter(i => i.type === 'element') as Element[]
const linesToRemove: (Element | Text)[] = []
lines.forEach((line, idx) => {
let nodeToRemove: Element | undefined

for (const child of line.children) {
if (child.type !== 'element')
continue
const text = child.children[0]
if (text.type !== 'text')
continue

let replaced = false
text.value = text.value.replace(regex, (...match) => {
if (onMatch.call(this, match, line, child, lines, idx)) {
replaced = true
return ''
}
return match[0]
})
if (replaced && !text.value.trim())
nodeToRemove = child
}

if (nodeToRemove) {
line.children.splice(line.children.indexOf(nodeToRemove), 1)

// Remove if empty
if (line.children.length === 0) {
linesToRemove.push(line)
if (removeEmptyLines) {
const next = code.children[code.children.indexOf(line) + 1]
if (next && next.type === 'text' && next.value === '\n')
linesToRemove.push(next)
}
}
}
})

for (const line of linesToRemove)
code.children.splice(code.children.indexOf(line), 1)
},
}
}
87 changes: 49 additions & 38 deletions packages/transformers/src/utils.ts
Original file line number Diff line number Diff line change
@@ -1,58 +1,67 @@
import type { Element, Text } from 'hast'
import type { ShikiTransformer, ShikiTransformerContext } from 'shiki'

export function createCommentNotationTransformer(
/**
* Regex that matches code comments
*/
const regex = /((?:\/\/|\/\*|<!--|[#"']|--|%%|;{1,2}|%{1,2})\s*)(\S.*?)(-->|\*\/|$)/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if we should hard-code the regex, this makes the code less flexiable

Copy link
Contributor Author

@fuma-nama fuma-nama Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm do you mean to allow passing the entire regex, including the part for matching comments? (like the previous code)

I assume createCommentNotationTransformer is for matching comment notations, it sounds reasonable to me to add the regex for matching comment inside the utility. I can revert it if you wanted


/**
* Create a transformer to process comment notations
*
* @param name transformer name
* @param onMatch function to be called when found a comment in code, return the replaced text.
*/
export function createCommentNotationTransformerExperimental(
name: string,
regex: RegExp,
onMatch: (
this: ShikiTransformerContext,
match: string[],
commentText: string,
fuma-nama marked this conversation as resolved.
Show resolved Hide resolved
line: Element,
commentNode: Element,
lines: Element[],
index: number,
) => boolean,
removeEmptyLines = false,
) => string,
): ShikiTransformer {
return {
name,
code(code) {
const lines = code.children.filter(i => i.type === 'element') as Element[]
const linesToRemove: (Element | Text)[] = []
lines.forEach((line, idx) => {
let nodeToRemove: Element | undefined

for (const child of line.children) {
if (child.type !== 'element')
continue
const text = child.children[0]
if (text.type !== 'text')
continue

let replaced = false
text.value = text.value.replace(regex, (...match) => {
if (onMatch.call(this, match, line, child, lines, idx)) {
replaced = true
return ''
}
return match[0]
})
if (replaced && !text.value.trim())
nodeToRemove = child
}

if (nodeToRemove) {
line.children.splice(line.children.indexOf(nodeToRemove), 1)

// Remove if empty
if (line.children.length === 0) {
linesToRemove.push(line)
if (removeEmptyLines) {
const next = code.children[code.children.indexOf(line) + 1]
if (next && next.type === 'text' && next.value === '\n')
linesToRemove.push(next)
}
}
lines.forEach((line, lineIdx) => {
// comment should be at the end of line (last token)
const last = line.children.findLast(i => i.type === 'element') as Element | undefined

if (!last || last.children.length === 0)
return
const text = last.children[0]
if (text.type !== 'text')
return

let deleteComment = false

const isEmptyLine = line.children.length === 1
text.value = text.value.replace(regex, (_, prefix, text, end) => {
// no other tokens except the comment
const replaced = onMatch.call(this, text, line, last, lines,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the second arg text is coming from the match, why do we need to change it from match to text? Doesn't it make the utils less flexiable?

Instead of making a new function as breaking changes, couldn't we just allow onMatch to return a string instead of boolean?

Copy link
Contributor Author

@fuma-nama fuma-nama Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new match here refers to the entire comment (// [!code] highlight), but we also want to remove the comment if the comment content becomes empty, e.g. // .

so we pass the text instead ([!code] highlight), the transformer replaces comment content, which allows us to detect if the new text is empty. This can also improve the flexibility, and remove duplicated regex for matching code comments.

As you see, the createCommentNotationTransformer now has the algorithm for matching comments. We can improve it to support other comment syntaxes.

For the problem about breaking change, I think we could also create a wrapper over the original function, would like to hear your opinions about this.

// take the next line if the current line will be removed
isEmptyLine ? lineIdx + 1 : lineIdx)

if (replaced.trim().length === 0)
deleteComment = true

return prefix + replaced + end
})

if (!deleteComment)
return

if (isEmptyLine) {
linesToRemove.push(line)
}
else {
line.children.splice(line.children.indexOf(last), 1)
}
})

Expand All @@ -61,3 +70,5 @@ export function createCommentNotationTransformer(
},
}
}

export { createCommentNotationTransformer } from './utils-legacy'
2 changes: 1 addition & 1 deletion packages/transformers/test/fixtures/all/a.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading