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

feat(transformers): introduce matchAlgorithm option for new matching algorithm #835

Merged
merged 9 commits into from
Jan 20, 2025

Conversation

fuma-nama
Copy link
Contributor

@fuma-nama fuma-nama commented Nov 14, 2024

Description

This PR aims to redesign notation transformer utility and its usage to improve performance and make it more intuitive.

Changes

  • Parse comments separately and cache it into the data property of code nodes.
  • Notation transformers read and replace parsed comments.
  • Update original token, remove empty comment tokens and lines if necessary.

New Syntax

console.log("hello") // [!code highlight]

console.log("highlighted") // [!code highlight:2]
console.log("highlighted")

// the comment below will be removed
// [!code highlight:2]
console.log("highlighted")
console.log("highlighted")

console.log("combine") // [!code focus] [!code highlight]
console.log("combine") // Comment Allowed [!code focus] [!code highlight]

JSX

<div>
    <p>Hello World</p>
    {/* [!code highlight] */}
    <p>Highlighted</p>
</div>

Breaking

It only looks for the last few tokens of line to scan comments:

console.log("// [!code highlight]"); // will not work

It doesn't need multiple // to chain notations:

console.log("test") // [!code highlight] // [!code ++]

// will cause weird result like:
console.log("test") // //

Edge cases

for usage like:

// some text [!code highlight:2]
console.log("highlighted")
console.log("highlighted")

line 2-3 will be highlighted, but the original comment is kept.

Backward Compatibility

Added a legacy option to force legacy behaviors:

transformerNotationFocus({ legacy: true })
transformerNotationHighlight({ legacy: true })
transformerNotationErrorLevel({ legacy: true })

Linked Issues

#619 #770

Additional context

this PR will introduce breaking changes, I currently made a separate package to avoid the problems. Backward compatibility is possible (edit: supported), but it takes extra care and further discussion.

code implementations:

  • the createCommentNotationTransformer parses comment with parseComments and cache the result.

  • each parsed comment object contains related hast nodes, and info:

        /* hello world */
     |----|-----------|-|
     prefix   content  suffix
    
  • remove nodes when content became empty.

  • transformers using createCommentNotationTransformer still provides a regex, but the regex no longer need to match the comment

Copy link

netlify bot commented Nov 14, 2024

Deploy Preview for shiki-matsu ready!

Name Link
🔨 Latest commit 2660901
🔍 Latest deploy log https://app.netlify.com/sites/shiki-matsu/deploys/678df18344914600084791b3
😎 Deploy Preview https://deploy-preview-835--shiki-matsu.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 14, 2024

Deploy Preview for shiki-next ready!

Name Link
🔨 Latest commit 2660901
🔍 Latest deploy log https://app.netlify.com/sites/shiki-next/deploys/678df18388cb310008147fce
😎 Deploy Preview https://deploy-preview-835--shiki-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@fuma-nama fuma-nama changed the title Notation transformers redesign breaking(transformers): redesign notation transformers Nov 14, 2024
if (comment.info[1].length === 0)
continue

const isLineCommentOnly = comment.line.children.length === (comment.isJsxStyle ? 3 : 1)
Copy link
Contributor Author

@fuma-nama fuma-nama Nov 14, 2024

Choose a reason for hiding this comment

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

it indicates the line contains only the comment itself, like:

// [!code highlight]
console.log("test")

so we will highlight the next line, and remove the original line when content became empty.

@@ -24,14 +24,14 @@ export function transformerNotationMap(

return createCommentNotationTransformer(
name,
new RegExp(`\\s*(?://|/\\*|<!--|#|--|%{1,2}|;{1,2}|"|')\\s+\\[!code (${Object.keys(classMap).map(escapeRegExp).join('|')})(:\\d+)?\\]\\s*(?:\\*/|-->)?\\s*$`),
new RegExp(`\\s*\\[!code (${Object.keys(classMap).map(escapeRegExp).join('|')})(:\\d+)?\\]`),
Copy link
Contributor Author

@fuma-nama fuma-nama Nov 14, 2024

Choose a reason for hiding this comment

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

we no longer need to provide the regex for matching comments, it is better given that the name createCommentNotationTransformer should handles this too.

@fuma-nama
Copy link
Contributor Author

fuma-nama commented Nov 14, 2024

The core changes are basically done.

I kept the failing unit tests for future experiments about backward compatibility, I'm thinking about a legacy option to support deprecated behaviours, going to make that this weekend

@fuma-nama fuma-nama marked this pull request as ready for review November 18, 2024 14:16
@Fro-Q
Copy link

Fro-Q commented Jan 18, 2025

This is what I'm looking for! Are you still working on it ? 'cause I actually need it so bad :>

@fuma-nama
Copy link
Contributor Author

See https://github.com/fuma-nama/shiki-transformers

@antfu antfu changed the title breaking(transformers): redesign notation transformers feat(transformers): introduce matchAlgorithm option for new design Jan 20, 2025
@antfu
Copy link
Member

antfu commented Jan 20, 2025

Hey @fuma-nama, thanks for the awesome work and sorry for the long due. In other to process this PR, we have to first make it non-breaking and rolling out it progressively. I have push some commits to introduce the matchAlgorithm which will default to the old behaviour, and we will rolling it by default in v3. I also wrote some guide for people to migrate or opt-in, if I messed anything feel free to push and improve it. Thanks

@antfu antfu changed the title feat(transformers): introduce matchAlgorithm option for new design feat(transformers): introduce matchAlgorithm option for new matching algorithm Jan 20, 2025
@antfu antfu merged commit ceca984 into shikijs:main Jan 20, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants