Feature: Adds realtime linting for markdown previewer.

ZachBaird commented 4 months ago

Because

There is a markdown preview page. It allows contributors to see how the markdown will render on the site, but does not indicate any issues that may exist with the markdown. When pull requests are created in curriculum, the linter detects these issues. The PR is prevented from merging, leading to additional commits satisfying linter conditions that the contributor was initially unaware of.

This PR

This PR adds linting to the site. As users preview their markdown, a linter is run against it based on the jsonc configuration file from curriculum. Any potential issues are listed below the preview for the user to resolve.

Their markdown is loaded up into a temporary file that the linter is run against. Results are saved and the file is deleted immediately after. Results are then formatted for presentation on the page.

Issue

Closes #4340

Pull Request Requirements

[x] I have thoroughly read and understand The Odin Project Contributing Guide
[x] The title of this PR follows the keyword: brief description of change format, using one of the following keywords:
- Feature - adds new or amends existing user-facing behavior
- Chore - changes that have no user-facing value, refactors, dependency bumps, etc
- Fix - bug fixes
[x] The Because section summarizes the reason for this PR
[x] The This PR section has a bullet point list describing the changes in this PR
[x] I have verified all tests and linters pass after making these changes.
[x] If this PR addresses an open issue, it is linked in the Issue section
[x] If applicable, this PR includes new or updated automated tests

ZachBaird commented 4 months ago

This PR may need the server to have the buildpacks set up differently. It should work with something like the following:

heroku buildpacks:add --index 1 heroku/nodejs
heroku buildpacks:add heroku/ruby

heroku buildpacks # Check that node.js is executed before ruby

MaoShizhong commented 4 months ago

How would we plan to account for custom rules, including changes that are made to them for whatever reason? We currently have 7, with 1 in review (which requires a disabling of MD048) and 2 assigned in other issues (one of which is also a custom version of a built-in just to provide a fixer for our npm scripts in the curriculum repo).

One of the things that comes to mind is that if we can't easily somehow get the web app to "use" the markdownlint files in the curriculum repo, and we basically need to "copy" the files over here, whether it would therefore be sensible to extract our linting for both repos to a new repo where we publish a custom NPM package? Then any updates to our linting can be done in that repo, and new releases could trigger a dependabot update for both repos. But of course, that route also has its own nuances and problems to consider.

ZachBaird commented 4 months ago

@MaoShizhong I had a lot of uncertainty around this as well. My initial thinking was:

If we include just the base configuration file, it allows us to give a true preview on the site. It won't lint for everything curriculum does, but would catch the major stuff. It also allows us to pretty much ship once we sort out this pipeline error.
Including the custom rules right away would not be difficult, but just copying them over and maintaining them in 2 places isn't great. I'm also unsure where best to put the files (perhaps in lib). This was something I was going to consult @KevinMulhern on once he had a chance to review this.
We may need to be cautious about adding too much linting in one go. It's unclear how much of a performance impact this could have on the server if the previewer sees a spike in usage.

I think publishing it as its own npm package is a really great idea.

MaoShizhong commented 4 months ago

@ZachBaird That's a very fair point about the performance impact we'd need to monitor.

I'm definitely not keen on the "duplicate the code and maintain in 2 places" thing, which was my first and biggest concern about feasability. That's fair though on the first point regarding not needing to lint everything. If people are fine with this then perhaps we could make it clear that the error list may not be exhaustive? I know 2 of the built-ins would cover 2 of the upcoming customs anyway, since those are customs purely because the built-ins don't have fixers but can have fixers, which would leave a potential 7 unchecked rules.

While I like the custom npm package idea, I am aware that'd mean we'd need to remind contributors that whenever they sync their fork's main with upstream, they should run npm update so they'd catch any package updates. While it seems relatively small to me, I'm aware this might cause some issues since custom markdownlint updates won't be brought along immediately by just syncing fork:main and upstream. Also issues with integrating with the VSCode markdownlint extension if the config gets packaged into a custom one and not laid bare in the individual repos

thatblindgeye commented 4 months ago

Having the linting stuff housed in its own repo has been mentioned before, so how strongly we feel about implementing this sort of feature may force us to prioritize such an effort since - as mentioned - having the same content in 2 repos is not ideal and requires more upkeep than necessary.

I'm probably on the fence of this integration. I think my main concern is whether the work required to get this working as ideally as possible makes sense in terms of how used the preview tool is. "Ideally as possible" would include transferring the linting content to its own repo/package as well as ensuring the preview tool can take into account custom rules/configs. The latter is important because if we based it solely on the base markdownlint config, it would most likely lead to confusion/contradiction when compared to our custom rules/config. Another thing to consider is that currently we have 2 configs: one for lessons and one for projects. So we'd need to be able to choose what config to run the preview against. Which if there's a worry about performance impact or we need to limit what we lint, is it even worth implementing at that point?

Another thing to consider is what the scope of the preview tool should be. Should it take into account linting stuff, or should the scope be kept literally, i.e. "This tool will preview your markdown, nothing more". Linting the markdown feels like it goes a little beyond just "previewing", but we can also possibly tweak the intent of the tool. Rather than just "preview", maybe "validate" or "preview and lint". Could also just be semantics at that point, though.

That said, for the users who may not yet know how to use Node (or maybe just don't want to go through that hassle and just want to quickly copy paste something, like I've done at times), this would be a great improvement to the current preview tool.

TheOdinProject / theodinproject