mdx-js / eslint-mdx

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

Parse error for multiline object props on React components #435

Closed mike-fam closed 12 months ago

mike-fam commented 1 year ago

Initial checklist

Affected packages and versions

2.0.5

Link to runnable example

https://github.com/mike-fam/eslint-mdx-multiline-prop-error

Steps to reproduce

  1. Create a new workspace with npm and install eslint with eslint-plugin-mdx
  2. Eslint configuration:
    {
    "overrides": [
        {
            "files": ["*.mdx"],
            "extends": "plugin:mdx/recommended"
        }
    ],
    "rules": {}
    }
  3. Create an mdx file with a react component with a multiline object prop e.g.
    <button
    style={{
        display: 'block'
    }}
    >
    Test
    </button>
  4. Lint the file, note the error: Parsing error: Expected value to be truthy

Expected behavior

The file should pass linting without any problems, as this is valid mdx syntax.

Actual behavior

Linting produces the following error: 0:0 error Parsing error: Expected value to be truthy

Runtime

Node v16, Node v14

Package manager

yarn v1

OS

macOS

Build and bundle tools

webpack

JounQin commented 1 year ago

Thanks for reporting issue by using the template, I'm on a vocation these days, I'll check later soon.

JounQin commented 1 year ago

It seems related to tab, if I change the source codes with spaces there will be no issue:

<button
  style={{
    display: "block",
  }}
>
  Test
</button>

The assertion error is thrown at https://github.com/mdx-js/eslint-mdx/blob/a1998c787f0049ae0cd705fc51f079c63ded75b2/packages/eslint-mdx/src/worker.ts#L491

I'll check how to fix this with tab


For text:

<button
    style={{
        display: 'block'
    }}
>
    Test
</button>

attrValue.data.estree.range is [16, 40], but it should be [16, 39] IMO, could it be an issue from micromark-extension-mdx-expression? @wooorm

I'll try to draft a simple reproduction for it.

mike-fam commented 1 year ago

Hi @JounQin, thanks for all your great work and investigating this. Is there any new update on this? For now, do you recommend replacing all the tabs in our stories with spaces or waiting for a new fix to be released?

morganfeeney commented 1 year ago

Hey @mike-fam did you manage to work around this one? I'm facing the same thing.

Ayc0 commented 1 year ago

I investigated a bit this issue.

With the following code:

-   list

    <Components data={`
    `} />

I also have the error Parsing error: Expected value to be truthy

This crashes because of this line: https://github.com/mdx-js/eslint-mdx/blob/9331895e902e4eb08cf2343bd7ccac2adf83af40/packages/eslint-mdx/src/worker.ts#L413.

When adding console.logs before and after attrValStart = prevCharOffset(attrValStart - 1); attrValEnd = nextCharOffset(attrValEnd);, here is what I have:

image

I think that the reason of the offset is because when writing <Component data={`\n `} />, acorn returns this node:

image

But the offsets start & end on the data attribute (32, 35) don't match the right start & end (end should have been 32, 38):

image

I'm not sure why the spaces are dropped in this estree (which comes from this processor https://github.com/mdx-js/eslint-mdx/blob/9331895e902e4eb08cf2343bd7ccac2adf83af40/packages/eslint-mdx/src/worker.ts#L79).

mike-fam commented 1 year ago

Hi @JounQin @wooorm sorry for bothering you. Is there any update on this issue?

remcohaszing commented 1 year ago

If I’m correct, this repo is mainly maintained by @JounQin. However, we haven’t heard from him in a while.

Maybe we should create and pin a Looking for maintainers issue?

JounQin commented 1 year ago

As I and @Ayc0 discovered above, it is not an internal issue but due to acorn return incorrect location info, but I'm not sure if it comes from acorn itself or micromark-extension-mdx-expression instead because the location info is restored at

https://github.com/micromark/micromark-extension-mdx-expression/blob/88c175f9ac684efc9306e23642807657df71d6a6/packages/micromark-util-events-to-acorn/dev/index.js#L216

cc @wooorm

mike-fam commented 1 year ago

@JounQin @wooorm Would you guys mind creating an issue in either acorn or micromark-extension-mdx-expression, since I know little context to how the eslint-mdx plugin is implemented and how it uses those dependencies

JounQin commented 1 year ago

@wooorm Sorry to ping again, but I think we need your help here.

wooorm commented 1 year ago

I dunno where this problem is, I’d appreciate a reduced test case. If it isn’t in eslint-mdx, I’d appreciate a test case that doesn’t use eslint-mdx?

JounQin commented 1 year ago

Working on a minimal reproduction without eslint-mdx.

JounQin commented 1 year ago

@wooorm Please check https://github.com/JounQin/test/blob/mdx-tab/test.mjs#L29-L32

Besides, https://github.com/mdx-js/eslint-mdx/issues/435#issuecomment-1382644367 could be another range issue not related to tab but list.


I tried acorn-jsx directly, there is no range issue:

/**
 * @typedef {import('estree').Program} Program
 * @typedef {import('estree-jsx').JSXElement} JSXElement
 * @typedef {import('estree-jsx').JSXAttribute} JSXAttribute
 */

const root =
  // @ts-expect-error
  /** @type {Program} */ (acorn.Parser.extend(acornJsx()).parse(contents, {
    ecmaVersion: 2020,
  }))
const statement = root.body[0]
if (typeof statement === 'object' && statement.type === 'ExpressionStatement') {
  const jsxElement = /** @type {JSXElement} */ (statement.expression)
  const attribute =
    /** @type {JSXAttribute} */ (jsxElement.openingElement.attributes[0])
  console.log(attribute.value)
}
Node {
  type: 'JSXExpressionContainer',
  start: 15,
  end: 21,
  expression: Node {
    type: 'TemplateLiteral',
    start: 16,
    end: 20,  // correct
    expressions: [],
    quasis: [ [Node] ]
  }
}
wooorm commented 1 year ago

@JounQin did you see my above commit?

JounQin commented 1 year ago

@wooorm Thanks! It seems working to me now. @mike-fam @morganfeeney @Ayc0 Can you help to confirm? You may need to delete your npm-lock.json or yarn.lock, etc to upgrade all related packages to confirm.

Ayc0 commented 1 year ago

I think that this is working for me too! Thank you very much! 🎉

(with yarn 2+, you can run yarn up -R micromark-util-events-to-acorn to upgrade micromark-util-events-to-acorn in sub-dependencies without having to remove the whole yarn.lock)