search

mdx-js / eslint-mdx

ESLint Parser/Plugin for MDX
https://npmjs.org/eslint-plugin-mdx
MIT License
264 stars 30 forks source link

[v2] `no-unused-expressions` in jsx content #384

Closed JounQin closed 2 years ago

JounQin commented 2 years ago

Initial checklist

Affected packages and versions

eslint-mdx

Link to runnable example

No response

Steps to reproduce

https://github.com/mdx-js/eslint-mdx/pull/383/files#diff-7dc63d54389fc5b805a5cebe74578027e4837d2eb08085555c7792fad8da8552R219

Expected behavior

No such error

Actual behavior

error

Runtime

Node v16

Package manager

yarn v1

OS

macOS

Build and bundle tools

No response

wooorm commented 2 years ago

I don’t understand the problem yet 🤔 What’s the input, and actual/expected output?

JounQin commented 2 years ago

I don’t understand the problem yet 🤔 What’s the input, and actual/expected output?

Like described in #380,


<Story>
  {{
    template: /* HTML */ ``,
  }}
</Story>

The { template: '' } results no-unused-expressions error for now which should be unexpected.

wooorm commented 2 years ago

Ah okay, that’s not a bug in MDX!

JounQin commented 2 years ago

@wooorm Like comments, do you think is there any possible to get tokens with correct loc? I'm using acornOptions#onToken now, but obviously their locations are incorrect. 🤣

See https://github.com/JounQin/test/blob/mdx-loc/index.js as reproduction.

wooorm commented 2 years ago

Yeah, we could do it: we could support onComment and onToken as arrays/functions (although I still see no reason to support onComment, but we could, and it makes more sense to have it if we support onToken).

If someone passes an array/function, we’d pass a different function, which fixes the position, and then adds the result to the given array or calls the given function?

JounQin commented 2 years ago

@wooorm

If someone passes an array/function, we’d pass a different function, which fixes the position, and then adds the result to the given array or calls the given function?

Yeah, that's what I'm thinking.

I have a simple thought on how to implement the basic function and it's working:

const acornOptions = options.acornOptions

const onToken = acornOptions && acornOptions.onToken

const tokenHandler = Array.isArray(onToken)
  ? token => onToken.push(token)
  : onToken

if (tokenHandler) {
  // will be deleted later, it's for getting correct loc for `tokens` by reusing the following `visitor` as `nodes`
  estree._tokens = tokens.map(token => ({
    ...token,
    type: 'fake', // `string` required for `estree-util-visit`
    _type: token.type,
  }))
}

visit(estree /*  */)

if (tokenHandler) {
  estree._tokens.forEach(({ _type, ...token }) => {
    token.type = _type
    tokenHandler(token)
  })
}

delete estree._tokens

What do you think? If you agree, I'd like to continue on https://github.com/micromark/micromark-extension-mdx-expression/pull/4

JounQin commented 2 years ago

@wooorm It seems jsx tokens are not presented in acorn#tokens:

See https://github.com/JounQin/test/blob/mdx-loc/index.js

<Story>
  {{
    template: /* HTML */ ``,
  }}
</Story>
// tokens
[
  {
    "type": {
      "label": "{",
      "beforeExpr": true,
      "startsExpr": true,
      "isLoop": false,
      "isAssign": false,
      "prefix": false,
      "postfix": false,
      "binop": null
    },
    "start": 11,
    "end": 12,
    "loc": {
      "start": {
        "line": 2,
        "column": 3,
        "offset": 11
      },
      "end": {
        "line": 2,
        "column": 4,
        "offset": 12
      }
    },
    "range": [11, 12]
  },
  {
    "type": {
      "label": "name",
      "beforeExpr": false,
      "startsExpr": true,
      "isLoop": false,
      "isAssign": false,
      "prefix": false,
      "postfix": false,
      "binop": null
    },
    "value": "template",
    "start": 17,
    "end": 25,
    "loc": {
      "start": {
        "line": 3,
        "column": 4,
        "offset": 17
      },
      "end": {
        "line": 3,
        "column": 12,
        "offset": 25
      }
    },
    "range": [17, 25]
  },
  {
    "type": {
      "label": ":",
      "beforeExpr": true,
      "startsExpr": false,
      "isLoop": false,
      "isAssign": false,
      "prefix": false,
      "postfix": false,
      "binop": null,
      "updateContext": null
    },
    "start": 25,
    "end": 26,
    "loc": {
      "start": {
        "line": 3,
        "column": 12,
        "offset": 25
      },
      "end": {
        "line": 3,
        "column": 13,
        "offset": 26
      }
    },
    "range": [25, 26]
  },
  {
    "type": {
      "label": "`",
      "beforeExpr": false,
      "startsExpr": true,
      "isLoop": false,
      "isAssign": false,
      "prefix": false,
      "postfix": false,
      "binop": null
    },
    "start": 38,
    "end": 39,
    "loc": {
      "start": {
        "line": 3,
        "column": 25,
        "offset": 38
      },
      "end": {
        "line": 3,
        "column": 26,
        "offset": 39
      }
    },
    "range": [38, 39]
  },
  {
    "type": {
      "label": "template",
      "beforeExpr": false,
      "startsExpr": false,
      "isLoop": false,
      "isAssign": false,
      "prefix": false,
      "postfix": false,
      "binop": null,
      "updateContext": null
    },
    "value": "",
    "start": 39,
    "end": 39,
    "loc": {
      "start": {
        "line": 3,
        "column": 26,
        "offset": 39
      },
      "end": {
        "line": 3,
        "column": 26,
        "offset": 39
      }
    },
    "range": [39, 39]
  },
  {
    "type": {
      "label": "`",
      "beforeExpr": false,
      "startsExpr": true,
      "isLoop": false,
      "isAssign": false,
      "prefix": false,
      "postfix": false,
      "binop": null
    },
    "start": 39,
    "end": 40,
    "loc": {
      "start": {
        "line": 3,
        "column": 26,
        "offset": 39
      },
      "end": {
        "line": 3,
        "column": 27,
        "offset": 40
      }
    },
    "range": [39, 40]
  },
  {
    "type": {
      "label": ",",
      "beforeExpr": true,
      "startsExpr": false,
      "isLoop": false,
      "isAssign": false,
      "prefix": false,
      "postfix": false,
      "binop": null,
      "updateContext": null
    },
    "start": 40,
    "end": 41,
    "loc": {
      "start": {
        "line": 3,
        "column": 27,
        "offset": 40
      },
      "end": {
        "line": 3,
        "column": 28,
        "offset": 41
      }
    },
    "range": [40, 41]
  },
  {
    "type": {
      "label": "}",
      "beforeExpr": false,
      "startsExpr": false,
      "isLoop": false,
      "isAssign": false,
      "prefix": false,
      "postfix": false,
      "binop": null
    },
    "start": 44,
    "end": 45,
    "loc": {
      "start": {
        "line": 4,
        "column": 2,
        "offset": 44
      },
      "end": {
        "line": 4,
        "column": 3,
        "offset": 45
      }
    },
    "range": [44, 45]
  }
]
wooorm commented 2 years ago

Those are handled by MDX. Not by Acorn. acorn#tokens represents what acorn parses. Not what MDX parses. Acorn handles JSX inside JavaScript, so if you put tags inside the expression, it should work. MDX handles JSX inside MDX, it doesn’t expose tokens. Similarly, a paragraph in the MDX is not present in acorn#tokens either!

JounQin commented 2 years ago

So it seems I need to transform mdxJsxFlowElement into token by myself then.

wooorm commented 2 years ago

You can do that. It depends on what you want to do and how far you want to take it. Do you want to act as if paragraphs and block quotes were written as a JS form? There are some small differences between MDX and JSX. And between JSX in MDX and JSX in JS.

JounQin commented 2 years ago

Personally, I'd like to only add least and necessary tokens for mdxJsxFlowElement/mdxJsxAttribute/mdxJsxAttributeValueExpression/mdxJsxTextElement.

JounQin commented 2 years ago

For

<Story style={{ width: '100%', flex: 1 }}>
  <>
    {{
      template: /* HTML */ ``,
    }}
    <div>Hello</div>
  </>
</Story>

The Fragment and div are not in tokens neither.

wooorm commented 2 years ago

Yes, because they're still MDX. Not JS. JavaScript in MDX only exists in braces or as import/exports. Acorn is only used for that JavaScript.

JounQin commented 2 years ago

That's OK. I'll transform them manually.

JounQin commented 2 years ago

This issue is not easy to fix as I thought, not only tokens are missing(I've added them back locally), the AST are also mismatched.

<Story>
  {{
    template: /* HTML */ ``,
  }}
</Story>

The AST from remark-mdx is:

[
  {
    "type": "ExpressionStatement",
    "expression": {
      "type": "ObjectExpression",
      "start": 11,
      "end": 45,
      "loc": {
        "start": {
          "line": 2,
          "column": 3,
          "offset": 11
        },
        "end": {
          "line": 4,
          "column": 3,
          "offset": 45
        }
      },
      "range": [
        11,
        45
      ],
      "properties": [
        {
          "type": "Property",
          "start": 17,
          "end": 40,
          "loc": {
            "start": {
              "line": 3,
              "column": 4,
              "offset": 17
            },
            "end": {
              "line": 3,
              "column": 27,
              "offset": 40
            }
          },
          "range": [
            17,
            40
          ],
          "method": false,
          "shorthand": false,
          "computed": false,
          "key": {
            "type": "Identifier",
            "start": 17,
            "end": 25,
            "loc": {
              "start": {
                "line": 3,
                "column": 4,
                "offset": 17
              },
              "end": {
                "line": 3,
                "column": 12,
                "offset": 25
              }
            },
            "range": [
              17,
              25
            ],
            "name": "template"
          },
          "value": {
            "type": "TemplateLiteral",
            "start": 38,
            "end": 40,
            "loc": {
              "start": {
                "line": 3,
                "column": 25,
                "offset": 38
              },
              "end": {
                "line": 3,
                "column": 27,
                "offset": 40
              }
            },
            "range": [
              38,
              40
            ],
            "expressions": [],
            "quasis": [
              {
                "type": "TemplateElement",
                "start": 39,
                "end": 39,
                "loc": {
                  "start": {
                    "line": 3,
                    "column": 26,
                    "offset": 39
                  },
                  "end": {
                    "line": 3,
                    "column": 26,
                    "offset": 39
                  }
                },
                "range": [
                  39,
                  39
                ],
                "value": {
                  "raw": "",
                  "cooked": ""
                },
                "tail": true
              }
            ]
          },
          "kind": "init"
        }
      ]
    },
    "start": 11,
    "end": 45,
    "loc": {
      "start": {
        "line": 2,
        "column": 3,
        "offset": 11
      },
      "end": {
        "line": 4,
        "column": 3,
        "offset": 45
      }
    },
    "range": [
      11,
      45
    ]
  }
]

And compare with acorn itself, they are very different, because remark-mdx treat the nested ObjectExpression as ExpressionStatement, the jsx element is not presented in the AST, this means the new AST will not be compatible with a lot of eslint plugins. What a pity.

wooorm commented 2 years ago

Both are ObjectExpression. In MDX and in Acorn. I don’t understand what you expected?

JounQin commented 2 years ago

Both are ObjectExpression. In MDX and in Acorn. I don’t understand what you expected?

@wooorm

Story JSX related estree nodes are not in the estree AST. I have to read them from mdast and transform them manually, like https://github.com/mdx-js/eslint-mdx/pull/394/files#diff-28c9951bdd4b9fbbb77321aec606c4ec820dc169ca472ad8991aa553bc4283baR374

wooorm commented 2 years ago

Yeah. Indeed. Because those JSX tags are part of what MDX parses itself. It’s parsed together with markdown things. Otherwise interleaving would be impossible. We can’t use Acorn for that because then we can’t have interleaving.

JounQin commented 2 years ago

Luckily, I transformed them manually successfully. 😉