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

Unterminated template when parsing template literals with multiple newlines (regression from v1) #1945

Closed
4 tasks done
shilman opened this issue Feb 21, 2022 · 5 comments
Closed
4 tasks done
Labels
💪 phase/solved Post is done

Comments

@shilman
Copy link

shilman commented Feb 21, 2022

Initial checklist

Affected packages and versions

@mdx-js/mdx

Link to runnable example

https://stackblitz.com/edit/github-rumgbj?file=index.js

Steps to reproduce

Add an extra \n between .foo and .bar styles in the linked stackblitz example

Expected behavior

It should compile successfully. We are using this pattern in Storybook's starter template:

https://raw.githubusercontent.com/storybookjs/storybook/next/lib/cli/src/frameworks/common/Introduction.stories.mdx

Actual behavior

It will crash with

Could not parse expression with acorn: Unterminated template

Or perhaps this is an intentional breaking change in MDXv2?

Runtime

No response

Package manager

No response

OS

No response

Build and bundle tools

No response

@shilman shilman changed the title Could not parse expression with acorn: Unterminated template Unterminated template when parsing template strings with multiple newlines (regression from v1) Feb 21, 2022
@shilman shilman changed the title Unterminated template when parsing template strings with multiple newlines (regression from v1) Unterminated template when parsing template literals with multiple newlines (regression from v1) Feb 21, 2022
@wooorm
Copy link
Member

wooorm commented Feb 25, 2022

Heya! Hmm, two things. First, your abbreviated problem:

Hi <style>{

`}</style> there!

That won’t work and is expected: https://mdxjs.com/docs/what-is-mdx/#interleaving. In markdown, those first are two paragraphs. And well, you can’t have that:

<p>Hi <style>{</p>
<p>`}</style> there!</p>

Second, is not completely “expected”, but also not “not expected”, following to the interleaving section:

<stuff>{

more

}</stuff>

Can you do this?

<stuff>
  {

    more

  }
</stuff>

@shilman
Copy link
Author

shilman commented Feb 28, 2022

Hi @wooorm thanks so much for getting back to me.

I updated the example to make it a little more readable and realized it was not exactly what I wanted to convey (though the behavior is the same).

Original (broken)

<style>{`
  .foo {}

  .bar {}
`}</style>

The "original" version worked with MDXv1 and we shipped it to tens of thousands of users as part of the example template in Storybook Docs. Now in MDXv2 it's broken. It feels like a regression to me, but if that's the expected behavior I'm totally fine with it -- I realize that's what major versions are for.

Updated1 (works)

<style>{`
  .foo {}
  .bar {}
`}</style>

Updated2 (also works)

<style>
  {`
    .foo {}
  
    .bar {}
  `}
</style>

Discussion

Both updated versions work. I could update our template accordingly, and provide migration instructions or potentially even ship a codemod to update users' code accordingly.

Before I do that, however, I want to confirm:

  • That this original example should not work in MDXv2, i.e. it's expected behavior and not a regression
  • Do you have a preference between Updated1 and Updated2? (I'm guessing Updated2 is more idiomatic MDX, but not entirely sure)

Thanks again for your help! And congratulations on the v2 release--super excited to get this into users' hands!!!! 🎉

@wooorm
Copy link
Member

wooorm commented Mar 16, 2022

Thanks for your patience. This is indeed expected behavior. Updated2 is what I recommend, yeah. The braces start and end a “block” by being on their own lines.

I’m still thinking of whether there’s a good reason and technical possibility to allow your original code anyway, I’m leaning towards it, not 100% sure yet though.

Thanks for all your kind words :)

@brendonco

This comment was marked as off-topic.

@wooorm
Copy link
Member

wooorm commented Apr 8, 2022

@brendonco you’re probably better off asking questions about storybook betas in storybook repos

endwaa added a commit to domstolene/designsystem that referenced this issue Apr 27, 2022
* Breaking after upgrading to MDXv2
* Related to mdx-js/mdx#1945
endwaa added a commit to domstolene/designsystem that referenced this issue Apr 27, 2022
* Breaking after upgrading to MDXv2
* Related to mdx-js/mdx#1945
endwaa added a commit to domstolene/designsystem that referenced this issue Apr 27, 2022
* Breaking after upgrading to MDXv2
* Related to mdx-js/mdx#1945
endwaa added a commit to domstolene/designsystem that referenced this issue Apr 27, 2022
* Breaking after upgrading to MDXv2
* Related to mdx-js/mdx#1945
endwaa added a commit to domstolene/designsystem that referenced this issue Apr 29, 2022
* Breaking after upgrading to MDXv2
* Related to mdx-js/mdx#1945
endwaa added a commit to domstolene/designsystem that referenced this issue Apr 29, 2022
* Breaking after upgrading to MDXv2
* Related to mdx-js/mdx#1945
endwaa added a commit to domstolene/designsystem that referenced this issue Apr 29, 2022
* Breaking after upgrading to MDXv2
* Related to mdx-js/mdx#1945
endwaa added a commit to domstolene/designsystem that referenced this issue May 2, 2022
* Breaking after upgrading to MDXv2
* Related to mdx-js/mdx#1945
endwaa added a commit to domstolene/designsystem that referenced this issue May 2, 2022
* Breaking after upgrading to MDXv2
* Related to mdx-js/mdx#1945
endwaa added a commit to domstolene/designsystem that referenced this issue May 2, 2022
* Breaking after upgrading to MDXv2
* Related to mdx-js/mdx#1945
endwaa added a commit to domstolene/designsystem that referenced this issue May 3, 2022
* Breaking after upgrading to MDXv2
* Related to mdx-js/mdx#1945
endwaa added a commit to domstolene/designsystem that referenced this issue May 3, 2022
* Breaking after upgrading to MDXv2
* Related to mdx-js/mdx#1945
endwaa added a commit to domstolene/designsystem that referenced this issue May 4, 2022
* Breaking after upgrading to MDXv2
* Related to mdx-js/mdx#1945
endwaa added a commit to domstolene/designsystem that referenced this issue May 4, 2022
* Breaking after upgrading to MDXv2
* Related to mdx-js/mdx#1945
wooorm added a commit to micromark/micromark-extension-mdx-expression that referenced this issue Oct 19, 2023
@wooorm wooorm added the 💪 phase/solved Post is done label Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

No branches or pull requests

3 participants