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

MdxFlowExpression doesn't seem to preserve white space in values #150

Open
bnchi opened this issue Oct 14, 2024 · 8 comments · May be fixed by #152
Open

MdxFlowExpression doesn't seem to preserve white space in values #150

bnchi opened this issue Oct 14, 2024 · 8 comments · May be fixed by #152

Comments

@bnchi
Copy link
Contributor

bnchi commented Oct 14, 2024

Given this test case :

from("  {`\n    a\n  `}", &ParseOptions::mdx()).unwrap()

the generated markdown tree seem to be missing the white spaces between the first \n and a

The tree outptut from the above Rust code is :

Root { children: [MdxFlowExpression { value: "`\na\n`", position: Some(1:3-3:5 (2-15)), stops: [(0, 3), (1, 4), (2, 9), (3, 10), (4, 13)] }], position: Some(1:1-3:5 (0-15)) }

I ran the same input on the JS version and the output is :

{
  type: 'root',
  children: [
    {
      type: 'mdxFlowExpression',
      value: '`\n  a\n`',
      position: [Object]
    }
  ],
  position: {
    start: { line: 1, column: 1, offset: 0 },
    end: { line: 3, column: 5, offset: 15 }
  }
}
@ChristianMurphy
Copy link
Collaborator

Good to see you again @bnchi! 👋
IIRC white space is not considered significant in a JSX context (E.G. https://eslint.style/rules/default/jsx-child-element-spacing)
In other words if could be interesting to investigate if you want, but this shouldn't break any tooling as far as I'm aware.

Did this come up from doing property testing comparing the JS and rust implementations?
Or is there another edge case this uncovers?

@bnchi
Copy link
Contributor Author

bnchi commented Oct 15, 2024

Hey @ChristianMurphy !

This is taken from a round trip test in the mdx implementation here : https://github.com/syntax-tree/mdast-util-mdx-expression/blob/main/test.js#L535C25-L535C47

Shouldn't I expect the round trip to not lose information like this when I go md -> mdast -> md ?

@wooorm
Copy link
Owner

wooorm commented Oct 15, 2024

Re Christian: This is different from JSX: this is expressions.

Expressions often have backticked strings. In them, there needs to be some persisting of whitespace.

On the other hand, people do often want to indent their expressions. Just like other things in markdown:

{
  `
    what about whitespace
    for this?
  `
}

There is a more complex case where expressions are used in JSX:

<alpha
  some-long-attributes={true}
  some-more-long-attributes={true}
  some-value={`
    what about whitespace
    for this?
  `}
/>

What you are seeing is code from markdown-rs and micromark/mdast-util* being out of sync with each other. Code such as micromark/micromark-extension-mdx-expression@103af9a would need to be backported. See also the issue mdx-js/mdx#2533 and the commit micromark/micromark-extension-mdx-jsx@67f9baf!

@bnchi
Copy link
Contributor Author

bnchi commented Oct 16, 2024

@wooorm does this need to be back-ported for the serialization feature you think ?

For now we can just serialize the trees as they come in.

@wooorm
Copy link
Owner

wooorm commented Oct 16, 2024

I am not entirely sure. There is handling in different places. Parsing and serializing. Regular expressions and expressions in JSX attributes. I think it’s these 4 places. Not sure which of them are out of date with the JS version, and thus which need to be backported

@bnchi
Copy link
Contributor Author

bnchi commented Oct 16, 2024

Round-trip testing will probably help us to know where the issues are as I move along since we were able to detect this one this way.

I'll see if I can make a PR to close this issue before continuing to work on the serialization process.

@wooorm
Copy link
Owner

wooorm commented Oct 16, 2024

Ah, I think I understand better what you mean. Yes: I do think that is needed! Appreciated, thank you for working on this!

@ChristianMurphy
Copy link
Collaborator

Round-trip testing will probably help us to know where the issues are as I move along since we were able to detect this one this way.

agreed, I added some ideas in #127 (comment)
and this may be further supported by #149

@bnchi bnchi linked a pull request Oct 17, 2024 that will close this issue
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 a pull request may close this issue.

3 participants