doczjs / docz

✍ It has never been so easy to document your things!
https://docz.site
MIT License
23.55k stars 1.46k forks source link

SyntaxError: Expected corresponding JSX closing tag #1691

Closed lparedesl closed 2 years ago

lparedesl commented 2 years ago

Getting SyntaxError: Expected corresponding JSX closing tag error when using any component with children inside the Playground component.

Using components that self close, for example <MyComponent /> works fine.

Here is a sample with the error. Please click on Components > Alert on the left menu.

https://codesandbox.io/s/docz-example-xj522

Thanks

panvourtsis commented 2 years ago

i also tried other examples and all have the same error ....

panvourtsis commented 2 years ago

today i used the example below to add typescript support and all and it worked.

https://codesandbox.io/s/9pucf

If you delete the yarn.lock though and rerun the yarn install it will again break.

tanbowensg commented 2 years ago

same problem

lparedesl commented 2 years ago

today i used the example below to add typescript support and all and it worked.

https://codesandbox.io/s/9pucf

If you delete the yarn.lock though and rerun the yarn install it will again break.

Thank you. I will try that

bartlomiej-kochanowicz-landingi commented 2 years ago

any update?

iandeherdt commented 2 years ago

Same issue

panvourtsis commented 2 years ago

couldn't find what caused it.

gmonte commented 2 years ago

Same issue here.


<Playground>
  {() => {
    const foo = 'bar'
    return <div />
  }}
</Playground>

Screen Shot 2022-01-21 at 18 30 52


<Playground>
  {() => {
    const foo = 'bar'
    return <div></div>
  }}
</Playground>

Screen Shot 2022-01-21 at 18 30 33


<Playground>
  <div />
</Playground>

Screen Shot 2022-01-21 at 18 31 26


<Playground>
  <div></div>
</Playground>

Screen Shot 2022-01-21 at 18 31 13


I have the same behavior when I use with/without a function body. It is working just with auto-closing tags.

My env: docz: 2.3.1 node: 16.13.1 yarn: 1.22.17 typescript: 4.4.2

Elgeneinar commented 2 years ago

Same issue here. For me it seems to be cutting out the first ending tag, so

<Playground>
<div> </div>
<p> </p>
</Playground>

becomes

<Playground>
<div>
<p> </p>
</Playground>
adbayb commented 2 years ago

After investigation, it seems that forcing (through yarn resolution or whatever) the downgrade of the package @babel/traverse to v7.16.7 (or @babel/core to v7.16.7) solves the issue. The regression was introduced for versions greater or equal than v7.16.8.

It might be related to this issue https://github.com/babel/babel/issues/14139 and this update https://github.com/babel/babel/pull/14105

nicolo-ribaudo commented 2 years ago

Hi! Babel maintainer here.

I'd be happy to help figure out if this is a Babel bug, or if it's a bug somewhere else that was hidden in this specific circumstance and unveiled by that Babel update.

Does docz use any custom Babel plugin to transform <Playground> tags? I only found https://github.com/doczjs/docz/tree/main/other-packages/babel-plugin-export-metadata but it looks unrelated.

adbayb commented 2 years ago

Hello @nicolo-ribaudo 👋 , First of all, thanks for your time and involvement 🙏 .

I've mentioned the https://github.com/babel/babel/pull/14105 since the issue starts to appear from @babel/traverse@v7.16.8 and above (same issue also with @babel/traverse@7.16.10). From the changelog, I can see that the only change introduced since was https://github.com/babel/babel/pull/14105.

Does docz use any custom Babel plugin to transform tags?

I'm not an active Docz maintainer but from what I see, It seems:

Capture d’écran 2022-01-31 à 09 11 38

Maybe the maintainers @pedronauck / @renatobenks can confirm.

nicolo-ribaudo commented 2 years ago

After a very long debug session (it took a few hours because I was not familiar with the project :grimacing:), I came to the conclusion that:


What is the fixed Babel bug exactly?

Babel has a few utility methods to control how the AST is traversed. One of them if path.stop(), whose goal is to enterly stop the current traversal. You can read its documentation in our semi-official plugins handbook.

However, in some circumstances it only prevented part of the remaining nodes from being traversed, rather than all of them. We fixed this bug, and now path.stop() behaves properly.


What is the docz bug?

docz has some code to remove the <Playground>...</Playground> tags, transforming

<Playground>
  <Alert>Some text here</Alert>
</Playground>

into

  <Alert>Some text here</Alert>

This is the utility function that should do that transformation: https://github.com/doczjs/docz/blob/259898c25838c052048bd81277d0d74d0ee5c1e2/core/docz-utils/src/jsx.ts#L13-L18

For completeness, here is the codeFromNode function: https://github.com/doczjs/docz/blob/259898c25838c052048bd81277d0d74d0ee5c1e2/core/docz-utils/src/ast.ts#L7-L28

codeFromNode takes a "condition" function, and returns a function that, when given the input code, returns a portion of the code whose AST matches the condition.

const open = codeFromNode(p => p.isJSXOpeningElement()); open(code) returns the the code of something that matches p.isJSXOpeningElement().

In our example above, both <Playground> and <Alert> match that condition: which one does it return? If we look at the valueFromTraverse implementation, it:

Calling path.stop() as soon as something matches the condition means that it returns the first node that matches it. When looking for a JSXOpeningElement it thus returns <Playground>, while when looking for a JSXClosingElement it's </Alert>.

(It might help looking at the AST for my example, remembering that Babel traverses nodes in "source code order": when it comes to JSX, it first traverses the opening tag, then the children and then the closing tag.)

After getting <Playground> and </Alert>, removeTags removes them from the input code.

We were "lucky" before because path.stop() didn't work properly in that case, so it didn't stop the traversal when it found a closing tag but continued normally (reaching </Playground> at the end of the traversal).


My suggested fix

Rather than finding an opening and a closing tag separately, I suggest getting them at the same time. Also, the current .replace-based approach is quite fragile. For example (even before the @babel/traverse fix), this didn't work:

<Playground>
  This is followed by a comment! { /* </Playground> */ }
  <Alert>Some text here</Alert>
</Playground>

because by replacing <Playground> and </Playground> with an empty string you get:

  This is followed by a comment! { /*  */ }
  <Alert>Some text here</Alert>
</Playground>

I suggest to rewrite removeTags like this:

const getTagContentsRange = valueFromTraverse(
  p => p.isJSXElement(),
  ({ node }) => {
    if (!node.closingElement) {
      // if the JSX element doesn't have a closingElement, it's because it's self-closed
      // and thus does not have any content: <Playground />
      return [0, 0];
    }
    return [node.openingElement.end, node.closingElement.start];
  }
);

export const removeTags = (code: string) => { 
  const [start, end] = getTagContentsRange(code);
  return code.slice(start, end);
};

by doing so we get the start/end location of the contents of the first JSX tag, which should be safe.

nicolo-ribaudo commented 2 years ago

I ended up creating a PR with the above fix - https://github.com/doczjs/docz/pull/1696

renatobenks commented 2 years ago

great work @nicolo-ribaudo, we're going to merge and release it as soon as we can