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

shilman commented 2 years ago

Initial checklist

[X] I read the support docs
[X] I read the contributing guide
[X] I agree to follow the code of conduct
[X] I searched issues and couldn’t find anything (or linked relevant results below)

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

wooorm commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Any idea?

You did not set any plugins, parser, or stringifier. Right now, PostCSS does nothing. Pick plugins for your case on https://www.postcss.parts/ and use them in postcss.config.js.
99% done plugins webpack-hot-middlewarewebpack built preview 685ff6eaca974a9b46f6 in 38315ms
ModuleBuildError: Module build failed:\node_modules\@storybook\mdx2-csf\node_modules\estree-to-babel\lib\estree-to-babel.js:108
    node.raw = raw || node.extra?.raw;
                                 ^

SyntaxError: Unexpected token '.'
    at wrapSafe (internal/modules/cjs/loader.js:1071:16)
    at Module._compile (internal/modules/cjs/loader.js:1121:27)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1177:10)
    at Module.load (internal/modules/cjs/loader.js:1001:32)
    at Function.Module._load (internal/modules/cjs/loader.js:900:14)
    at Module.require (internal/modules/cjs/loader.js:1043:19)
    at require (internal/modules/cjs/helpers.js:77:18)

wooorm commented 2 years ago

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

mdx-js / mdx