search

mdx-js / eslint-mdx

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

[v2] incorrect comment loc info #380

Closed JounQin closed 2 years ago

JounQin commented 2 years ago

Initial checklist

Affected packages and versions

2.0.0.next.1

Link to runnable example

No response

Steps to reproduce

{/* First */}

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

print with console.log(JSON.stringify(node.data?.estree.comments || [], null, 2))

Expected behavior

The second comment should start with line: 5, column: 14 and end with line: 5, column: 24

Actual behavior

[
  {
    "type": "Block",
    "value": " First ",
    "start": 1,
    "end": 12,
    "loc": {
      "start": {
        "line": 1,
        "column": 1
      },
      "end": {
        "line": 1,
        "column": 12
      }
    },
    "range": [
      1,
      12
    ]
  }
]
[
  {
    "type": "Block",
    "value": " HTML ",
    "start": 42,
    "end": 52,
    "loc": {
      "start": {
        "line": 4,
        "column": 19
      },
      "end": {
        "line": 4,
        "column": 29
      }
    },
    "range": [
      42,
      52
    ]
  }
]

Runtime

Node v16

Package manager

yarn v1

OS

macOS

Build and bundle tools

No response

JounQin commented 2 years ago

@wooorm Do you have any idea? The node is unist Node from remark-* at https://github.com/mdx-js/eslint-mdx/blob/master/packages/eslint-mdx/src/parser.ts#L89

The worst thing is the wrong comment loc may block ESLint process(I didn't find out why yet).

(I didn't try, but the body maybe incorrect at the same time.)

wooorm commented 2 years ago
JounQin commented 2 years ago

any reason you’re not on a stable v2?

2.0.0.next.1 is eslint-mdx

parsing of each “chunk” of JS-in-markdown happens from 1:1, and then a starting position as added onto it, I guess this is happening for nodes, but not for comments

So this is a known issue? Are you going to fix it?

wooorm commented 2 years ago

I can imagine it being a problem. And if it’s an issue for you, then it’s now a known problem :)

Do you want to investigate? It might be useful for the future to get your hands at micromark-extension-mdx*? But: I don’t mind doing it either, it’s probably faster if I do it!

JounQin commented 2 years ago

OK, I will give it a try today.

JounQin commented 2 years ago

@wooorm See https://github.com/micromark/micromark-extension-mdx-expression/pull/2

wooorm commented 2 years ago

Perhaps you can make a small reproduction of this problem? (I don’t see a problem with comment positions in micromark-extension-mdx-expression)

JounQin commented 2 years ago

@wooorm See https://github.com/JounQin/test/tree/mdx-loc

branch mdx-loc, simply run npm test to see comments loc info.

wooorm commented 2 years ago

The first comment is correct, right? Do you have a problem with the second comment?

Btw, I don’t think the code makes sense? Why is there an object as the child of Story

wooorm commented 2 years ago

Something fishy going on, because the earlier value affects the second comment:

{/* First */}

<Story>
  {{
    template: /* HTML */ ``,
  }}
</Story>
{
  type: 'Block',
  value: ' HTML ',
  start: 42,
  end: 52,
  loc: {
    start: { line: 4, column: 19 },
    end: { line: 4, column: 29 }
  },
  range: [ 42, 52 ]
}
{}

<Story>
  {{
    template: /* HTML */ ``,
  }}
</Story>
{
  type: 'Block',
  value: ' HTML ',
  start: 31,
  end: 41,
  loc: {
    start: { line: 4, column: 19 },
    end: { line: 5, column: 24 }
  },
  range: [ 31, 41 ]
}
JounQin commented 2 years ago

The first comment is correct, right?

Should the column of unified node be 1-indexed? (acorn's is 0-indexed).

I forgot it's in estree.

Btw, I don’t think the code makes sense? Why is there an object as the child of Story

This is how storybook for Angular works.

wooorm commented 2 years ago

I’m not sure how I came up with the previous logic. I made a little algorithm locally that works and is much simpler. I’ll have to make the code pretty and add test it all, but should have it ready soon!

wooorm commented 2 years ago

Solved in https://github.com/micromark/micromark-extension-mdx-expression/releases/tag/micromark-util-events-to-acorn%401.0.5!

JounQin commented 2 years ago

@wooorm

import Basic from './basic'

export const meta={
  title: 'Blog Post',
}

# Blog Post

Lorem ipsum dolor sit amet, consectetur adipiscing **elit**. Ut ac lobortis <b>velit</b>.

{/* This is a comment */}

```css
@media (min-width: 400px) {
  border-color: #000;
}
{/* This is a comment */}

Subtitle

Lorem ipsum dolor sit amet, consectetur adipiscing elit. Ut ac lobortis velit.


```log
file:///Users/JounQin/Workspaces/GitHub/test/node_modules/micromark-util-events-to-acorn/index.js:213
    const column = lines[line].column + (pointInSource.column - 1)
                               ^

TypeError: Cannot read properties of undefined (reading 'column')
    at parseOffsetToUnistPoint (file:///Users/JounQin/Workspaces/GitHub/test/node_modules/micromark-util-events-to-acorn/index.js:213:32)
    at file:///Users/JounQin/Workspaces/GitHub/test/node_modules/micromark-util-events-to-acorn/index.js:168:26
    at visit (file:///Users/JounQin/Workspaces/GitHub/test/node_modules/estree-util-visit/index.js:85:37)
    at visit (file:///Users/JounQin/Workspaces/GitHub/test/node_modules/estree-util-visit/index.js:68:30)
    at eventsToAcorn (file:///Users/JounQin/Workspaces/GitHub/test/node_modules/micromark-util-events-to-acorn/index.js:148:5)
    at atEnd (file:///Users/JounQin/Workspaces/GitHub/test/node_modules/micromark-extension-mdxjs-esm/lib/syntax.js:144:22)
    at go (file:///Users/JounQin/Workspaces/GitHub/test/node_modules/micromark/lib/create-tokenizer.js:216:13)
    at main (file:///Users/JounQin/Workspaces/GitHub/test/node_modules/micromark/lib/create-tokenizer.js:202:9)
    at Object.write (file:///Users/JounQin/Workspaces/GitHub/test/node_modules/micromark/lib/create-tokenizer.js:132:5)
    at writeToChild (file:///Users/JounQin/Workspaces/GitHub/test/node_modules/micromark/lib/initialize/document.js:247:15)
image

It can be reproduced simply in my original reproduction.

wooorm commented 2 years ago

done