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

Change Request: Show red underlines in examples in rules docs #16227

Closed
1 task done
captbaritone opened this issue Aug 22, 2022 · 10 comments · Fixed by #18041
Closed
1 task done

Change Request: Show red underlines in examples in rules docs #16227

captbaritone opened this issue Aug 22, 2022 · 10 comments · Fixed by #18041
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects

Comments

@captbaritone
Copy link
Contributor

captbaritone commented Aug 22, 2022

ESLint version

N/A

What problem do you want to solve?

  • Make the example errors in rules docs easier to understand.
  • Have the rules docs more directly demonstrate the user experience of enabling the rule (what you see in the docs mirrors what you will see in your editor)
  • (optional) Surface actual lint error messages in the example code on hover.

What do you think is the correct solution?

Implement a custom syntax highlighter (inspired by shiki-twoslash) which, at build time, optionally runs code through ESLint and augments the syntax-highlighted HTML with diagnostics. This could be implemented as code directly in ESLint, or as a standalone npm module pulled in as a dependency.

Participation

  • I am willing to submit a pull request for this change.

Additional comments

I've been playing with a version of this locally and I have something which works, but it will need quite a bit more work to get it robust enough to be able to fully replace the existing Prism-based syntax highlighter.

Screen Shot 2022-08-22 at 2 23 15 PM

Thanks to @orta who has been looking into this with me.

@captbaritone captbaritone added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Aug 22, 2022
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Aug 22, 2022
@snitin315
Copy link
Contributor

Implement a custom syntax highlighter (inspired by #16227)

I suppose you linked the wrong URL here.

@captbaritone
Copy link
Contributor Author

captbaritone commented Aug 23, 2022

Implement a custom syntax highlighter (inspired by #16227)

I suppose you linked the wrong URL here.

Fixed

@nzakas
Copy link
Member

nzakas commented Aug 26, 2022

Ooh this would be nice! We were actually planning on developing a custom element that was basically a mini playground to use in the docs, but if there’s something that can work already, I’m all for it.

@nzakas nzakas moved this from Needs Triage to Ready to Implement in Triage Aug 26, 2022
@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Aug 26, 2022
@captbaritone
Copy link
Contributor Author

@nzakas Is there a timeline on the mini playground? Is the plan to use that for the code examples in the rules docs? If the mini playground would replace this solution in the near-ish future, it might not be worth the effort of building this static version.

@nzakas
Copy link
Member

nzakas commented Sep 19, 2022

The mini playground will happen after the new config system is finalized and shipped, so no strict timeline but likely won’t be until next year.

@fasttime
Copy link
Member

@captbaritone are you still interested in pushing this forward? Do you need any help?

@kecrily
Copy link
Member

kecrily commented Jan 25, 2024

Does it need to be automatic? Or should we mark it manually?

@nzakas
Copy link
Member

nzakas commented Jan 26, 2024

@kecrily it needs to be automatic, otherwise it's just too much documentation to do by hand.

@captbaritone
Copy link
Contributor Author

If there's a more sophisticated long-term plan for the docs that's even more interactive, it's probably not worth building this just as an interim solution since it will likely prove rather complex. I took a stab at building a version for my personal blog. You can see it in action here: https://jordaneldredge.com/blog/interesting-bugs-caught-by-eslints-no-constant-binary-expression/

With code here: https://github.com/captbaritone/jordaneldredge.com/blob/master/lib/eslintShiki.js

It works, but I don't think it's 100% robust.

Some things that made it prove tricky:

It needs to couple tightly with syntax highlighting, which generally constructs a list of tagged tokens or token ranges and then wraps each token in a tag. However, our underline logic must wrap those tag (potentially more than one, and potentially forcing a split of a token). This forces the token stream to not be a flat list of tags, but nested, which introduces complexity.

Additionally, if I recall, the syntax highlighter we currently use might not expose the token stream in such a way that we could even intercept it. That means we may need to change syntax highlighters.

But maybe someone who likes interesting algorithm challenges would like to take this on! I don't think that person is me at this point in time.

I agree with @nzakas that it would have to be automatic to be worth it.

@fasttime
Copy link
Member

Thanks for the links to the blog and the repo @captbaritone, this previous work is very useful as a reference. As you can see, there is now a PR to implement this change: #18041.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
Archived in project
Triage
Ready to Implement
Development

Successfully merging a pull request may close this issue.

6 participants