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

normalize class property #9723

Merged
merged 5 commits into from
Jan 22, 2024
Merged

Conversation

blackmann
Copy link
Contributor

Changes

Attempting to use some of the common transformers raises an exception:

MDXError: classValue.replace is not a function
    at TransformContext.transform (node_modules/@astrojs/mdx/dist/index.js:93:27)
    at async Object.transform (node_modules/vite/dist/node/chunks/dep-V3BH7oO1.js:64060:30)
    at async loadAndTransform (node_modules/vite/dist/node/chunks/dep-V3BH7oO1.js:49741:29)
    at async instantiateModule (node_modules/vite/dist/node/chunks/dep-V3BH7oO1.js:50759:10)

This is because addClassToHast returns an array of strings. But Arrays don't have a .replace. See: https://github.com/antfu/shikiji/blob/31241ba4dc47c2d4d7809ee08c6573b26b07b812/packages/shikiji-core/src/utils.ts#L44

Testing

I couldn't figure out how to write a unit test for this. But I edited a build version with this logic and it works.

Docs

Copy link

changeset-bot bot commented Jan 18, 2024

🦋 Changeset detected

Latest commit: 528b57a

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the feat: markdown Related to Markdown (scope) label Jan 18, 2024
@@ -129,6 +131,10 @@ export async function createShikiHighlighter({
};
}

function normalizeMaybeArray(value: Properties[string]): string | null {
return Array.isArray(value) ? value.join(' ') : (value as string);
Copy link
Member

Choose a reason for hiding this comment

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

If value is cast to string, when does the function return null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated @ematipico

@matthewp
Copy link
Contributor

What does this fix? Is there an issue or code example?

@blackmann
Copy link
Contributor Author

blackmann commented Jan 18, 2024

I gave a description with the PR; to elaborate, if you added a codeblock like the following to an .md content file:

  // &mut means mutable reference
  fn next_question(&mut self) -> &Question {
    let count = self.questions.len() - 1; // [!code --]
    if self.current_question().is_answered() && self.current_index < count {
      self.current_index += 1;
    }

    self.current_question()
  }

Notice on line 3, we do // [!code --], this triggers the transformerNotationDiff transformer. In that process, addClassToHast is called which converts the class of that line from string to an array of string.

This PR converts it back to a string by just concatenating them.

Here's a screenshot of the error

Screenshot 2024-01-18 at 7 10 30 PM

@matthewp

@blackmann
Copy link
Contributor Author

blackmann commented Jan 18, 2024

This is a patch follow-up to #9643 which exposes the transformers API from shikiji which was published earlier today.

Comment on lines 65 to 66
const classValue = normalizeMaybeArray(node.properties.class) ?? '';

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the fix! This makes sense to me. Could we also handle this for style? I assume transformers could also change that to a non-string now.

Maybe we can rename and generalize the function as normalizePropAsString

@blackmann
Copy link
Contributor Author

@bluwy @ematipico

ready

Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
@blackmann
Copy link
Contributor Author

@florian-lefebvre @bluwy @ematipico

Can we get this merged soon?

@ematipico ematipico merged commit 2f81cff into withastro:main Jan 22, 2024
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: markdown Related to Markdown (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants