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

Test case for multiline string attribute values #15

Conversation

bradleyayers
Copy link

@bradleyayers bradleyayers commented Oct 26, 2024

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

The initial 2 spaces are always trimmed from multiline string values in JSX attributes. I encountered this when trying to render code in MDX by doing something like this:

JSON example:

<JsonCodeBlock code={`
{
  foo: "bar"
}`} />

Rest of the page here…

What ends up happening is that the string is parsed as \n{\nfoo: "bar"\n} and the indentation is lost.

Examples in the wild:

This PR adds a test case for it, but I'm not sure the best approach to fixing this.

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Oct 26, 2024
@bradleyayers bradleyayers force-pushed the broken-jsx-attribute-multiline-strings branch from de59d52 to 6362ce1 Compare October 26, 2024 05:44
@bradleyayers bradleyayers force-pushed the broken-jsx-attribute-multiline-strings branch from 6362ce1 to 1594507 Compare October 26, 2024 05:47
bradleyayers added a commit to bradleyayers/nativewind that referenced this pull request Oct 26, 2024
The code examples in the comments were missing
indentation, e.g.

```
{
  marginTop: env(safe-area-inset-top)
}
```

would be rendered as

```
{
marginTop: env(safe-area-inset-top)
}
```

The trimming seems to be caused by
syntax-tree/mdast-util-mdx-jsx#15

Instead of waiting for an upstream fix this commit
refactors the examples to remove the indentation
and use CSS properties instead of JS objects.
@wooorm
Copy link
Member

wooorm commented Oct 26, 2024

We are dealing with ASTs here: many things don’t roundtrip exactly.
What is important is that the first serialized result, parsed again, and then serialized again, is the same.

That the first spaces are eaten is intentional: people want to indent their things. And that’s the solution: indent your things.
Why not indent things?

@remcohaszing
Copy link
Member

This isn’t about indentation / formatting. This is about a JavaScript template literal whose value isn’t parsed correctly. Whitespace inside strings matters.

@wooorm
Copy link
Member

wooorm commented Oct 26, 2024

Yes, it is, whitespace matters in markdown. Markdown is a whitespace sensitive language. This is markdown first. Then JavaScript.

@wooorm
Copy link
Member

wooorm commented Oct 26, 2024

Not good:

<x y={`
{
  foo: "bar"
}`} />

Good:

<x y={`
  {
    foo: "bar"
  }`}
/>

https://mdxjs.com/playground/

@wooorm
Copy link
Member

wooorm commented Oct 26, 2024

Whitespace inside strings matters.

*  > -    <x y={`
   >                how many spaces?`} />

@bradleyayers
Copy link
Author

bradleyayers commented Oct 26, 2024

Thanks for the link to https://mdxjs.com/playground/, that's a useful tool. Previously I had been using astexplorer.net (linked from https://v0.mdxjs.com/advanced/ast) as a reference but it's using mdxhast rather than mdast. I find mdxhast's parsing to match my expectation, where the start indentation of a node is used as the reference for subsequent lines https://astexplorer.net/#/gist/2befce6edce1475eb4bbec001356b222/ff4a11cb7688bd590efceae09d70064aed9e29ff

Here's the code I was comparing, I used <pre> so that it's easy to see the visual result in the playground.

# top level

<pre children={`
0
 1
  2`}></pre>

# nested in blockquote

> <pre children={`
> 0
>  1
>   2`}></pre>

# nested in list in blockquote

> - <pre children={`
>   0
>    1
>     2`}></pre>

@wooorm
Copy link
Member

wooorm commented Oct 28, 2024

You can configure the playground to show different things by selecting the select next to “Show”, markdown AST (mdast), HTML AST (hast), JS AST (estree). Yet. It all works the same.

AST Explorer seems out of date. The term mdxhast isn’t in use for years.

I recommend using our website: it is always up to date, and has many options. Plus, docs.

This behavior is intentional; there is much code in place to handle things this way. In MDX, you can indent JSX and expressions and ESM a bit, just like other things in markdown:

{
  `asd
  qwe
    zxc`
}

<x {
  ...{y: `asd
  qwe
    zxc`} } />

<x y={
  `asd
  qwe
    zxc`} />

Each time gives a string:

asd
qwe
   zxc

I find mdxhast's parsing to match my expectation, where the start indentation of a node is used as the reference for subsequent lines

That is not quite the case. Try indenting the <pres themselves. You will notice that this does not impact the string. Indent too much and you get a Babel error.

@ChristianMurphy
Copy link
Member

AST Explorer seems out of date. The term mdxhast isn’t in use for years.

It is, we're still waiting for MDX 2 to be merged for almost 2 years

fkling/astexplorer#689

Much less MDX 3

marklawlor pushed a commit to nativewind/nativewind that referenced this pull request Nov 1, 2024
The code examples in the comments were missing
indentation, e.g.

```
{
  marginTop: env(safe-area-inset-top)
}
```

would be rendered as

```
{
marginTop: env(safe-area-inset-top)
}
```

The trimming seems to be caused by
syntax-tree/mdast-util-mdx-jsx#15

Instead of waiting for an upstream fix this commit
refactors the examples to remove the indentation
and use CSS properties instead of JS objects.
@wooorm
Copy link
Member

wooorm commented Nov 1, 2024

Closing as intentional, thanks!

@wooorm wooorm closed this Nov 1, 2024
@wooorm wooorm added the 🙅 no/wontfix This is not (enough of) an issue for this project label Nov 1, 2024

This comment has been minimized.

@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Nov 1, 2024
@remcohaszing
Copy link
Member

Yes, it is, whitespace matters in markdown. Markdown is a whitespace sensitive language. This is markdown first. Then JavaScript.

The behaviour feels a bit unnatural at first, but with this explanation it makes perfect sense.

This does mean the text range between the JSX attribute expression start and end offsets isn’t always a valid JavaScript expression. There are probably some related bugs in MDX analyzer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙅 no/wontfix This is not (enough of) an issue for this project 👎 phase/no Post cannot or will not be acted on
Development

Successfully merging this pull request may close these issues.

4 participants