-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/mdx/mdx/rfsf2wfrp |
cc @omry Finally! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😻
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 |
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. |
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. |
I’m sorry, I didn’t mean to insult you. I don’t want to close the PR. |
@lex111 (@yangshun opened #902 after I ran into the issue). 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. @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? |
No problem, although it is strange to me that the tests in this case do not guarantee the correct. |
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! |
👏 |
Probably fixes #902
For example, this code block
Produces:
Before:
-> Uncaught ReferenceError: model is not defined
After:
->
python command subdir=\${model.nb_layers}
(same as input)Another example:
Produces:
Before:
->
$sut = (Split-Path -Leaf $MyInvocation.MyCommand.Path) -replace '\.Tests\.', '.' . "$here$sut
(missed slash ->$here$sut
, should be$here\$sut
)After:
->
$sut = (Split-Path -Leaf $MyInvocation.MyCommand.Path) -replace '\.Tests\.', '.' . "$here\$sut"
(same as input)