Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hi, I like this rule, why not PR to eslint-mdx? #1

Closed
JounQin opened this issue Mar 17, 2021 · 6 comments
Closed

Hi, I like this rule, why not PR to eslint-mdx? #1

JounQin opened this issue Mar 17, 2021 · 6 comments

Comments

@JounQin
Copy link

JounQin commented Mar 17, 2021

No description provided.

@ianlet
Copy link
Owner

ianlet commented Mar 17, 2021

Hi @JounQin ! Thanks for your message.

We could definitely make a PR to https://github.com/mdx-js/eslint-mdx but it would need more test cases to make sure it behaves correctly/as expected in most situations.

Also if I recall correctly, at the time I patched this there was a thread in eslint-mdx or mdx itself stating that the engine was being rewritten and that it will probably handle this issue by default (meaning it should support out of the box components without blank lines around/within it). I'll try to find it and link it here.

Knowing that, I didn't take time to make a PR to the repo, thinking it would have been resolved soon enough.

Edit: here's the issue mdx-js/mdx#628

It seems to have been resolved though! But I don't know if it was released yet and if eslint-mdx upgraded to the new version.

If it's the case then this patch is no longer useful ☺️

@JounQin
Copy link
Author

JounQin commented Mar 17, 2021

That's may be true for v2, but for now it's a good rule for v1.

@ianlet
Copy link
Owner

ianlet commented Mar 17, 2021

You're right!

Then would you mind trying different use cases with this rule and see if it breaks something or if something is not correctly linted/fixed? And open an issue/PR in this repo so I can addresses them?

Then I will fork the eslint-mdx repo to make a PR and add the rule directly there. Sounds good for you?

@JounQin
Copy link
Author

JounQin commented Mar 17, 2021

You've already had test cases, so I think it's fine to PR directly and add it to the recommended config as warning by default. We can't test all eage cases, that's OK. Don't worry bout it.

@ianlet
Copy link
Owner

ianlet commented Mar 19, 2021

Alright I'll make a PR later today then.

@JounQin
Copy link
Author

JounQin commented Mar 19, 2021

If you have any question about the source code or testing locally, feel free to ask me here.

@JounQin JounQin closed this as not planned Won't fix, can't repro, duplicate, stale Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants