JounQin commented 2 years ago

close #3

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)
[x] If applicable, I’ve added docs and tests

Description of changes

As title

wooorm commented 2 years ago

What's the reason for this? We now always put those comments on the programs, meaning you can already access them? And this even breaks that behavior?

wooorm commented 2 years ago

And the comments that are currently available (because this lib overwrites the option) have a correct location, where's with this option they wouldn't have a correct location?

JounQin commented 2 years ago

What's the reason for this? We now always put those comments on the programs, meaning you can already access them? And this even breaks that behavior?

I'm not sure why, but it seems not working for me at https://github.com/mdx-js/eslint-mdx/pull/383/files#diff-7ae7909b53d4d5541a50a53722ab76dcecb3fe8a1602b303cf48872d7db9643dR103

And what behavior is broken? I don't understand, estree.comments is still there.

JounQin commented 2 years ago

And the comments that are currently available (because this lib overwrites the option) have a correct location, where's with this option they wouldn't have a correct location?

It will be correct location because comments reuse acornOptions.onComment array, they're same instance. That's why I dropped the function type onComment.

JounQin commented 2 years ago

See https://github.com/mdx-js/eslint-mdx/runs/6081840545?check_suite_focus=true

It run without the patch.

https://github.com/mdx-js/eslint-mdx/actions/runs/2191072870

This one run successfully after patched this package.

https://github.com/mdx-js/eslint-mdx/blob/fix/comments/patches/micromark-util-events-to-acorn%2B1.0.6.patch

wooorm commented 2 years ago

Can you clarify what you need, I don’t understand? If you need comments, use mdastNode.data.estree.comments. The comments are there, with fixed positions. Allowing acornOptions.onComment like this would a) remove those comments, b) no longer fix their positions.

wooorm commented 2 years ago

I'm not sure why, but it seems not working for me at mdx-js/eslint-mdx#383 (files)

I don’t quite understand what’s going on around your code. Can you console.log node.data.estree? It should have comments without your patch, no?

Where are you passing onComment in your code? Can you replace it with node.data.estree.comments?

JounQin commented 2 years ago

Allowing acornOptions.onComment like this would a) remove those comments, b) no longer fix their positions.

It will not, see the test cases, and the positions are still correct. onComment and comments are the same instance in that case. Again, that's why I dropped the function type of onComment support.

JounQin commented 2 years ago

I don’t quite understand what’s going on around your code. Can you console.log node.data.estree? It should have comments without your patch, no?

@wooorm

The bad thing is the progress is blocked in this case (not sure why yet...)

However, the current changes are definitely compatible with previous behavior, and it makes us can get all comments directly instead of traverse all nodes.

See https://github.com/micromark/micromark-extension-mdx-expression/pull/4/files#diff-5bb8db779819ddef5956a5d9d5949c05ef7445237656ca37bf2f02720271440bR359-R376 and https://github.com/micromark/micromark-extension-mdx-expression/pull/4/files#diff-5bb8db779819ddef5956a5d9d5949c05ef7445237656ca37bf2f02720271440bR329-R346

JounQin commented 2 years ago

Just a thought, comments order maybe important for ESLint, and traversing comments from node.data.estree may result incorrect ordered comments due to parent/children nodes.

wooorm commented 2 years ago

You don’t need to traverse node.data.estree. The comments are already right there in the order that ESLint wants it: https://github.com/micromark/micromark-extension-mdx-expression/blob/72f9624ce033494b62094feffb40816006bc3f09/packages/micromark-util-events-to-acorn/dev/index.js#L140.

I don’t understand why you need onComment, because the comments are right there as an array?

wooorm commented 2 years ago

The bad thing is the progress is blocked in this case (not sure why yet...)

Maybe it helps to first solve the issue of being able to console.log in your code?

However, the current changes are definitely compatible with previous behavior

I am one step earlier: I do not understand why you need this feature. I don’t care yet how it is implemented.

and it makes us can get all comments directly instead of traverse all nodes.

I don’t understand this goal, I believe this is not needed currently to traverse nodes.

JounQin commented 2 years ago

@wooorm

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

We get unified root here, and traverse it to set body/comments before, what do you mean by because the comments are right there as an array? We need to get them from node of unified, and when traversing the order could be wrong (child first https://github.com/mdx-js/eslint-mdx/blob/master/packages/eslint-mdx/src/traverse.ts#L23). So like tokens, I'd like to collect them in acornOptions by ourselves. Of course, the position info has been corrected in this package.

I don’t understand this goal, I believe this is not needed currently to traverse nodes.

I mean traverse unified nodes. And I want to keep comments(onComment) like tokens(onToken).

wooorm commented 2 years ago

when traversing the order could be wrong (child first https://github.com/mdx-js/eslint-mdx/blob/master/packages/eslint-mdx/src/traverse.ts#L23)

This is your code. If that code is wrong, then your code is wrong? And you can change it? This is the normal unified/unist traverser: https://github.com/syntax-tree/unist-util-visit. It does not do it like that. It first calls node, then it does children.

So like tokens, I'd like to collect them in acornOptions by ourselves

There is a difference between comments and tokens: we do not expose tokens in the AST. We do not care about them. So you are free to track them. But we do expose comments in the AST as node.data.estree.comments. We fix their positions.

P.S. I now understand:

why you still need to traverse (because there are several nodes that have a node.data.estree, and you want to collect all comments in one array)
that this PR is adding support for onComment as an array only, not as a function, and that when acornOptions.onComment is a reference to an array, that reference is also stored as node.data.estree.comments. You are correct that the array is still on node.data.estree.comments too! However: it is buggy! You collect all comments in one array, of several node.data.estree’s, and because you pass one array as a reference, and that reference is then set on node.data.estree, the same array is stored on all programs.

github-actions[bot] commented 2 years ago

Hi! This was closed. Team: If this was merged, please describe when this is likely to be released. Otherwise, please add one of the no/* labels.

JounQin commented 2 years ago

@wooorm Thanks for reviewing, I changed the logic now.

codecov-commenter commented 2 years ago

Codecov Report

Merging #4 (6e7db82) into main (72f9624) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main        #4   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          619       695   +76     
=========================================
+ Hits           619       695   +76

Impacted Files	Coverage Δ
...ckages/micromark-util-events-to-acorn/dev/index.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 72f9624...6e7db82. Read the comment docs.

JounQin commented 2 years ago

This is your code. If that code is wrong, then your code is wrong? And you can change it?

Ohh... It seems right. I changed the order, it works as expected now!

However, I still think this PR is worth.

wooorm commented 2 years ago

I still think this PR is worth.

Why do you think it is useful?

Because you can already do what you want without it, with the AST, and unified likes doing things in ASTs when possible!

There is something to say for supporting all acorn options. But a) I don’t see a use case for this option, and I’d rather not add features that nobody needs b) this implementation does only 50%, arrays are supported, but the function form isn’t, and I’d rather not add a partial implementation.

JounQin commented 2 years ago

@wooorm Would you like to review this PR again?

wooorm commented 2 years ago

so I do have if (commentHandler/tokenHandler)...

You already do on L185, L231, L237, and in the functions.

Why should we implement another fixPositionOfToken?

Because your hacking around it by patching stuff on nodes that cause a bunch of @ts-expect-error and then delete stuff, which is slow.

And also, your proposed solution would increase the complexity which is complained by ESLint.

Then ignore that rule instead of all the @ts-expect-errors. fixPositionOfToken is now inline in the code on L212-L228. Move that to the outer scope?

JounQin commented 2 years ago

@wooorm How about the current implementation?

wooorm commented 2 years ago

Thanks! Landing in GH-5!

micromark / micromark-extension-mdx-expression

fix: support `acornOptions.onComment` and `acornOptions.onToken` #4

Initial checklist

Description of changes

Codecov Report