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: not evaluate code block starting with \$ #991

Merged
merged 1 commit into from
Mar 26, 2020

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Mar 23, 2020

Probably fixes #902

For example, this code block

```
python command subdir=\${model.nb_layers}
```

Produces:

Before:

<pre><code parentName="pre" {...{}}>{`python command subdir=\\${model.nb_layers}
      `}</code></pre>

-> Uncaught ReferenceError: model is not defined

After:

<pre><code parentName="pre" {...{}}>{`python command subdir=\\\$\{model.nb_layers}
      `}</code></pre>

-> python command subdir=\${model.nb_layers} (same as input)


Another example:

$sut = (Split-Path -Leaf $MyInvocation.MyCommand.Path) -replace '\.Tests\.', '.' . "$here\$sut"

Produces:

Before:

<pre><code parentName="pre" {...{}}>{`$sut = (Split-Path -Leaf $MyInvocation.MyCommand.Path) -replace '\\.Tests\\.', '.' . "$here\$sut"
      `}</code></pre>

-> $sut = (Split-Path -Leaf $MyInvocation.MyCommand.Path) -replace '\.Tests\.', '.' . "$here$sut (missed slash -> $here$sut, should be $here\$sut)

After:

<pre><code parentName="pre" {...{}}>{`$sut = (Split-Path -Leaf $MyInvocation.MyCommand.Path) -replace '\\.Tests\\.', '.' . "$here\\$sut"
      `}</code></pre>

-> $sut = (Split-Path -Leaf $MyInvocation.MyCommand.Path) -replace '\.Tests\.', '.' . "$here\$sut" (same as input)

@vercel
Copy link

vercel bot commented Mar 23, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/mdx/mdx/rfsf2wfrp
✅ Preview: https://mdx-git-fork-lex111-lex111-iss902.mdx.now.sh

@vercel vercel bot temporarily deployed to Preview March 23, 2020 19:04 Inactive
@ChristianMurphy ChristianMurphy requested a review from a team March 23, 2020 19:26
@ChristianMurphy ChristianMurphy added 🏡 area/internal This affects the hidden internals 🐛 type/bug This is a problem 💎 v1 Issues related to v1 🔍 status/open labels Mar 23, 2020
@yangshun
Copy link

cc @omry Finally!

Copy link
Member

@johno johno left a comment

Choose a reason for hiding this comment

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

😻

@wooorm
Copy link
Member

wooorm commented Mar 25, 2020

I think this looks great, but I believe (as people could use template interpolation) this will be technically breaking and therefore will have to wait for 2.0

@lex111
Copy link
Contributor Author

lex111 commented Mar 26, 2020

@wooorm but the other tests are passing, so nothing's broken? Especially this test that uses template interpolation, isn't that it? What other case of template interpolation could there be?

Then should I close this PR?

@wooorm
Copy link
Member

wooorm commented Mar 26, 2020

That the tests didn’t fail, and still don’t, may not be the proof that this is non-breaking!

I believe the escape function is only used in text nodes, so in the MD of MDX, whereas the test case you linked is the JSX in MDX.

I’m thinking of this (weird) behavior:

<div>

If you’re fine with a slash, this works: \${1 + 1}

</div>

Or <span>\${Math.PI}</span>.

Which with current MDX compiles to:

    <div>
      <p>{`If you’re fine with a slash, this works: \\${1 + 1}`}</p>
    </div>
    <p>{`Or `}<span>{`\\${Math.PI}`}</span>{`.`}</p>

It’s a bit weird though, so I’m curious to hear what others think.

@lex111
Copy link
Contributor Author

lex111 commented Mar 26, 2020

That the tests didn’t fail, and still don’t, may not be the proof that this is non-breaking!

Take it easy. I just meant that the tests should cover the use case that you mean.

In short, close PR if you want, I’m not interested in doing it anymore.

@wooorm
Copy link
Member

wooorm commented Mar 26, 2020

I’m sorry, I didn’t mean to insult you. I don’t want to close the PR.

@omry
Copy link

omry commented Mar 26, 2020

@lex111 (@yangshun opened #902 after I ran into the issue).
Thanks for fixing it!

I think @wooorm is just concerned that this may break compatibility and that it's best to land it for the next major version and not as a minor version fix.
It's a legitimate concern that should be evaluated, there is no need to get offended.

@wooorm, in my opinion that the example that you gave seems very unnatural and it seems very unlikely that someone would rely on it. why would anyone in their right mind add a backslash if they don't have to?

@lex111
Copy link
Contributor Author

lex111 commented Mar 26, 2020

No problem, although it is strange to me that the tests in this case do not guarantee the correct.
After all, we can apply new replacements for only fenced code blocks. Feel free to go further with this PR.

@johno
Copy link
Member

johno commented Mar 26, 2020

Considering it is an "accidental feature" and pretty unnatural to use I think we should be okay merging and releasing as a patch. This "functionality" hasn't been documented and isn't part of the spec.

Thank you very much for the PR, @lex111, and your thoughts all!

@johno johno merged commit f460b7e into mdx-js:master Mar 26, 2020
@bravo-kernel
Copy link

👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏡 area/internal This affects the hidden internals 💪 phase/solved Post is done 🐛 type/bug This is a problem 💎 v1 Issues related to v1
Development

Successfully merging this pull request may close these issues.

Unable to render \${...} in code block
8 participants