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

Configure prettier and eslint to treat DocsShowCode as code examples #861

Open
AlexVelezLl opened this issue Dec 9, 2024 · 14 comments · May be fixed by #905
Open

Configure prettier and eslint to treat DocsShowCode as code examples #861

AlexVelezLl opened this issue Dec 9, 2024 · 14 comments · May be fixed by #905
Assignees
Labels

Comments

@AlexVelezLl
Copy link
Member

AlexVelezLl commented Dec 9, 2024

Product

KDS.

Current behavior

When we want to show code examples in the KDS docs, we use the DocsShowCode component. We can show HTML, JavaScript, and CSS code with this component. For example:

<DocsShowCode language="javascript">
  console.log("Hello world")
</DocsShowCode>

The problem is that eslint and prettier treat the code inside DocsShowCode as HTML code, so eslint raises exceptions for linting rules and prettier formats this code as html instead of treating this as a code example. So always that we have a css or javascript code examples, we need to write <!-- eslint-disable --> and <!-- prettier-ignore --> comments around it for them to avoid exceptions and unwanted formatting. So we end up with something like:

<!-- eslint-disable -->
<!-- prettier-ignore -->
<DocsShowCode language="javascript">
  console.log("Hello world")
</DocsShowCode>
<!-- eslint-enable -->

Which adds unnecessary complexity to writing our code examples.

Desired behavior

We should configure prettier and eslint to at least ignore the code inside the DocsShowCode component as it's just a code example, and there is no need to apply linting rules and HTML formatting.

At the end, we should be able to write code examples with just:

<DocsShowCode language="javascript">
  console.log("Hello world")
</DocsShowCode>

without the need to write the <!-- eslint-disable --> and <!-- prettier-ignore --> comments.

The Value Add

We will be able to write code examples more easily. And we won't have to worry about unwanted formatting in our code examples.

Possible Tradeoffs

If prettier just ignores the code inside, it will be the responsibility of the developer to write the code example in a well-formatted way.

Additional notes

@rtibbles tried it and it was difficult

@EshaanAgg
Copy link
Contributor

EshaanAgg commented Jan 11, 2025

This seems difficult to crack (and thus pretty interesting, too!) I have no clue how to crack the same, but I would love to drop resources and links here that, according to me, might help facilitate the fix!

From what I've read, one way to solve the problem might be to implement a custom plugin on top of vue-eslint-parser for the same. The AST specification might be a good starting point. We can use AST Explorer with the language set to Vue and parser as vue-eslint-parser to get started!

Update 1:

If we are to follow the plugin route, the following implementation nodes might help create a basic demo:

Basic Plugin
  1. In node_modules, create a folder eslint-plugin-custom and create an index.js with the following content:
module.exports = {
    rules: {
        'no-lint-docs-show-code': {
            meta: {
                    type: "layout",
                    docs: {
                        description: "Disables the linting of the code examples in the documentation.",
                        recommended: true,
                    },
                },
                create(context) {
                    return context.parserServices.defineTemplateBodyVisitor({
                        VElement(node) {
                            if (node.rawName === "DocsShowCode") {
                                console.log(node)
                            }
                        },
                    });
                }
            }
        }
    }
  1. Add the following lines to the eslint.config.js:
esLintConfig.plugins.push('custom');
esLintConfig.rules['custom/no-lint-docs-show-code'] = "warn"

You can use commands like npx eslint ./docs/pages/ktable.vue. The plugin currently logs all the target DocsShowCode nodes. I am stuck because the context.report option has no API to disable all the other linting rules related to the particular AST node.

Update 2:

These examples' errors are due to using the vue/html rule in all documentation files. As far as I could find, the only way to configure a plugin to skip just a particular node of an AST is to "republish" the code of the plugin and alter its create function with the custom checks to identify the target AST nodes and skip the same, which I don't think is a practical solution.

Maybe we can create an ESLint plugin that can kind of "fix" these issues for us by inserting an eslint-disable and eslint-enable comment for us whenever the same is used in the documentation pages. It might be a crude fix, but it would be nice to have a feature that, if not completely, should substantially reduce the pain associated with these examples.

I am enclosing a sample script if someone from the team wants to try it!

Auto Insert ESLint enable and disable comments | Plugin Code
module.exports = {
  rules: {
    'no-lint-docs-show-code': {
      meta: {
        type: 'layout',
        fixable: 'code',
        docs: {
          description:
            'Disables the linting of the code examples in the documentation.',
          recommended: true,
        },
      },
      create(context) {
        const sourceCode = context.getSourceCode();
        const template =
          sourceCode.parserServices.getTemplateBodyTokenStore &&
          sourceCode.parserServices.getTemplateBodyTokenStore();

        // Finds the first comment token before/after the given node
        // Breaks the search if it encounters any token of a different type
        const findCommentToken = (node, searchFn, commentContent) => {
          while (node) {
            if (
              node.type === 'HTMLComment' &&
              node.value.trim() === commentContent
            ) {
              return true;
            }

            if (
              node.rowName === 'DocsShowCode' ||
              node.type === 'HTMLWhitespace'
            ) {
              node = searchFn(node, { includeComments: true });
            } else return false;
          }

          return false;
        };

        return context.parserServices.defineTemplateBodyVisitor({
          VElement(node) {
            if (node.rawName === 'DocsShowCode') {
              const openTag = template.getFirstToken(node);
              const closeTag = template.getLastToken(node);

              if (!openTag || !closeTag) {
                return;
              }

              // There should be a "eslint-disable" comment before the opening tag
              const hasPrevComment = findCommentToken(
                openTag,
                template.getTokenBefore,
                'eslint-disable',
              );
              if (!hasPrevComment) {
                context.report({
                  node,
                  loc: openTag.loc.start,
                  message:
                    'Code examples in the documentation should not be linted. Add an eslint-disable comment before the opening tag of the same.',
                  fix: (fixer) =>
                    fixer.insertTextBefore(
                      openTag,
                      '<!-- eslint-disable -->\n',
                    ),
                });
              }

              // There should be a "eslint-enable" comment after the closing tag
              const hasNextComment = findCommentToken(
                closeTag,
                template.getTokenAfter,
                'eslint-enable',
              );
              if (!hasNextComment) {
                context.report({
                  node,
                  loc: closeTag.loc.end,
                  message:
                    'Code examples in the documentation should not be linted. Add an eslint-enable comment after the closing tag of the same.',
                  fix: (fixer) =>
                    fixer.insertTextAfter(closeTag, '\n<!-- eslint-enable -->'),
                });
              }
            }
          },
        });
      },
    },
  },
};

@AlexVelezLl
Copy link
Member Author

AlexVelezLl commented Jan 14, 2025

Hey @EshaanAgg! Good to hear from you again! I definitely think that a solution for this will include some AST hacking! Although keep in mind that node_modules is in our git ignore file, so if we manually add something there, we wont be able to commit it :).

Would be great if you can open a draft PR including the findings you've already had. And I can assign this to you if you are interested in continuing to work on this issue :). Just let us know 🤗.

@rtibbles
Copy link
Member

The way to do this without writing to node_modules would be for us to create a packages directory and then use the workspaces functionality of yarn (npm and pnpm also support this) to handle these internal packages. This is what we do in the Kolibri repo.

Note that the eslint part of the problem is probably the easier - getting prettier to either ignore the text, or properly format it as code is the harder part.

@EshaanAgg
Copy link
Contributor

Hi @AlexVelezLl @rtibbles! It feels great to be back! I would love to work on this issue!

The way to do this without writing to node_modules would be for us to create a packages directory and then use the workspaces functionality of yarn (npm and pnpm also support this) to handle these internal packages. This is what we do in the Kolibri repo.

Even I had tried the same earlier, but yarn allows workspaces to be configured only for private packages that are not meant to be published to npm, AFAIK. If there is some workaround for this, I would love to learn about it! (Since I wasn't aware of anything like it and wanted a quick way to try out the AST parsing, I devised this quick and dirty way.)

Note that the eslint part of the problem is probably the easier - getting prettier to either ignore the text, or properly format it as code is the harder part.

In my experience, suppressing the eslint warnings is enough! (As the vue/html causes the error, which is unrelated to prettier). But would be happy to test it out a bit more!

@rtibbles
Copy link
Member

yarn allows workspaces to be configured only for private packages that are not meant to be published to npm

We have both private and public packages in the yarn workspace in the Kolibri repository.

suppressing the eslint warnings is enough

Prettier runs on the entire component though - so if you don't add prettier ignore, it will reformat code in the blocks. This is why we have all the prettier-ignore comments, because otherwise the code in the DocsShowCode components get reformatted as if they were plain text.

@rtibbles
Copy link
Member

I have assigned you, nevertheless :)

@EshaanAgg
Copy link
Contributor

EshaanAgg commented Jan 14, 2025

Prettier runs on the entire component though - so if you don't add prettier ignore, it will reformat code in the blocks. This is why we have all the prettier-ignore comments, because otherwise the code in the DocsShowCode components get reformatted as if they were plain text.

Ahh, that makes sense! Thanks for the clarification.

As a status update, would like to mention that I have now kind of worked out the edge cases related to the adding of the eslint-enable and eslint-disable comments before and after the DocsShowCode component. Much like Eslint, Prettier has no option to selectively not format some particular AST nodes, and the current way forward to me seems to be that we would need to add a prettier-ignore comment as well to all such examples.

But this approach has a couple of major problems:

  • The cluttering of the code does not reduce this way but rather gets "automated," as our plugins now add these comments for us.
  • The plugins sometimes add the comments unnecessarily. Like from docs/pages/releases, we have many instances of the DocsShowCode component that look like the following:
<DocsShowCode language="javascript">
  // imports KButton version 3.0.0-alpha (next unstable) 
  import KModal from 'kolibri-design-system/lib/KButton/next';
</DocsShowCode>

These components are fine as they are and do not raise any prettier or eslint errors themselves! If we make such a rule, we will also blindly surround them with these comments.

  • We would need to ensure that the lint action now always runs before the format action, as the lint action would be responsible for adding the appropriate comments and preventing the auto-formatting of the text content. While this can be enforced in CI/CD, making all the developers work on the same codebase is an added nuisance to getting everything working.
  • Having prettier add the prettier-ignore comments and eslint add the eslint-disable comments can solve the above-mentioned point, but the same feels like a lot of complexity for something rather simple!

Considering all these factors, I strongly feel that trying to hack at this at the plugin level seems like an uphill battle that might not be for the best (until we have a better approach than just adding these disabling comments). I saw that we are planning to rework the documentation comments a bit (as in #845), which led me to think maybe we can refactor the API of these components to address these problems.

What I would like to propose is to adopt a more Vuetify-like documentation API, where all the examples live in separate folders and as independent complete Vue files (refer to one such example). Then the documentation components loads these files, separated their template, CSS and JS, (can use simple RegEx for the same as in this component) and displays them in different tabs!

This approach has some obvious advantages:

  • All the current oddities of lining and formatting that we are dealing with now vanish, as all the code is auto-formatted and lined using the existing rules.
  • Since the documentation code would exactly match what is displayed, the chances of errors in the documentation would decrease drastically, and the review process for new documentation would become easier as the examples could be reviewed, tested, and worked upon individually.

The only major con of this approach I can think of is that we would need to dedicate considerable effort towards this migration! But I do believe that there would be many open source enthusiasts that would be willing to help us out in the same if we proceed with the same, so the same shouldn't be as daunting as it seems :)

Do you think this can be a good way to tackle this?

@MisRob
Copy link
Member

MisRob commented Jan 16, 2025

@EshaanAgg thank you for looking into this, we appreciate it.

I agree that the original approach we wanted to explore here may end up causing more trouble than benefit.

I think separating the examples to separate files, that lint could simply ignore, could be a way to go and as you mention it has another benefits. In the case of very large documentation pages, this would help with their maintenance. I was just actually planning to extract examples from the KCard docs to separate files. What you suggest would be a step further towards standardized way of doing it.

One thing to note - as you have noticed, we don't always show the full code for each example and I think it's important to retain this behavior to prevent from clutter. So we should still be able to tweak the displayed code snippet to whatever we wish, even if it wasn't strictly
functional.

Also I'd suggest to plan this work iteratively, where in the first version, we would use DocsExample, but placed in a separate file. After we have successfully migrated at least some places, we could then discuss more improvements in line with your further suggestions.

Let me mention this on Slack and invite more team members so we can see if we're all on the same board. If there's an agreement, would you still be interested in this work? And after we have some basis, I agree that refactoring the pages would indeed be many good first issues for the community.

@rtibbles
Copy link
Member

I think this provides a reasonable alternative.

If your snippet is very short and it's too much overhead to put in a separate file, you can just add the ignore statements and move on.

If your snippet is long and it could benefit from being properly code formatted (which will be interpreted properly by prettier), you can put it in a separate file and have it be inlined.

@MisRob
Copy link
Member

MisRob commented Jan 16, 2025

If your snippet is very short and it's too much overhead to put in a separate file, you can just add the ignore statements and move on.

@rtibbles Ah yes! If we could retain both ways that'd be great and the migration would be much easier.

@EshaanAgg
Copy link
Contributor

If there's an agreement, would you still be interested in this work?

Yes, most definitely! As @rtibbles proposed, having the DocsShowExample component support both types of inline sources and external files seems like an excellent middle ground that we can opt into!

@MisRob
Copy link
Member

MisRob commented Jan 20, 2025

Thank you @EshaanAgg. So far I've only heard positive feedback from the team. Let me do final confirmation tomorrow during the design system circle meeting and then I think this could be ready for work.

@MisRob
Copy link
Member

MisRob commented Jan 22, 2025

Hi @EshaanAgg, so everybody is on board with this plan. You're welcome to proceed with the parts I commented on in #861 (comment). Just note that DocsExample is not yet available, so for now we will keep using DocsShow/DocsShowCode.

@MisRob
Copy link
Member

MisRob commented Jan 22, 2025

@EshaanAgg Thanks again for thinking carefully and improving the way we do things!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants