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

Conversation

fuma-nama
Copy link
Contributor

@fuma-nama fuma-nama commented Aug 22, 2024

Description

Start counting from next line of the comment if the current line is empty.

For example:

// [!code highlighted:2]
highlighted
highlighted
highlighted // [!code highlighted:2]
highlighted

Linked Issues

#619

Additional context

I made some changes to the design of createCommentNotationTransformer so it matches the comment token without a given regex, I've documented it in case for other contributors who doesn't understand it well.

note: forgot to reset local repo after the last PR was merged, apologies for the messy commit history.

Copy link

netlify bot commented Aug 22, 2024

Deploy Preview for shiki-matsu failed.

Name Link
🔨 Latest commit d994692
🔍 Latest deploy log https://app.netlify.com/sites/shiki-matsu/deploys/66daa107edebba00088c46e9

Copy link

netlify bot commented Aug 22, 2024

Deploy Preview for shiki-next failed.

Name Link
🔨 Latest commit d994692
🔍 Latest deploy log https://app.netlify.com/sites/shiki-next/deploys/66daa10771cf290008252980

@antfu antfu self-requested a review August 22, 2024 16:39
@fuma-nama
Copy link
Contributor Author

fuma-nama commented Aug 23, 2024

Also I realized this is a good opportunity to improve the current behaviors of notation transformers.

Like:

Normal

console.log("hello") // [!code xxx] original comment

Multiple transformers

console.log("hello") // [!code highlight] [!code world:hello] original comment

(maybe we can support the previous // original comment // [!code highlight] // [!code world:hello] syntax?)

impact next line if the current line is empty (this PR)

// [!code highlight] [!code world:hello]
console.log("hello") // original comment

keep current line

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

[!code current] will keep and highlight the empty line. I think it would be required once this PR gets merged.

it requires some extra work but won't cause a break change except the current proposal of "Ignoring empty lines"

@fuma-nama
Copy link
Contributor Author

fuma-nama commented Sep 3, 2024

@antfu I'm fixing some edge cases and I found one of the test cases:

function hello(indentSize, type) {
  if (indentSize === 4 && type !== 'tab') {
    	console.log('Each next indentation will increase on 4 spaces'); // [!code error] // [!code focus]
  }
}

gives the mismatch:

image

I thought it was the problem of my changes, but it seems like the original snapshot was wrong.
The original snapshot didn't apply the error transformer, and my fix accidentally fixed it.

I'll commit the changes for now (it's probably a breaking change, although docs didn't mention that explicitly)

/**
* 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

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.

@fuma-nama
Copy link
Contributor Author

fuma-nama commented Oct 25, 2024

I'm doing some experiments on notation transformers, and I found it's possible to redesign the current algorithm of notation transformers to support JSX comments e.g. #770, and optimize performance by reading only the last few tokens of each line.
Parsed comments can be cached per codeblock so chaining multiple notation transformers can lose less performance.

I putted these changes here, but this introduces breaking change as some backward incompatible changes are made, including "Ignore empty lines on notation transformer range" (the aim of this PR). I'm thinking to port the changes to a PR, if you're up to it.

@antfu
Copy link
Member

antfu commented Nov 14, 2024

Feel free to always create the PR (if you already work on it), so we can discuss about the implementations.

Sorry I was taking a long day offs, need to catch up and review a lot things

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.

2 participants