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

Rule proposal: Prevent stray syntax from ending up in front of users #3586

Open
captbaritone opened this issue Jun 12, 2023 · 6 comments
Open

Comments

@captbaritone
Copy link
Contributor

JSX syntax makes it very easy to leave stray syntax behind while updating code. For example, say you have:

return <h1>Hello!</h1>;

And then realize you want to wrap it in some other element. You might accidentally end up with:

return (
  <Wrapper>
    <h1>Hello!</h1>;
  </Wrapper>
);

Which will leave a stray semicolon in your user-facing UI.


Does a rule already exist here, or elsewhere, to prevent that type of bug?

If not, I wrote one which looks like this:

const SUSPECT_STRINGS = new Set([';', '}', ')}', ');']);

module.exports = {
  create(context) {
    return {
      'JSXText, Literal'(node) {
        if (
          (node.type !== 'JSXText' && node._babelType !== 'JSXText') ||
          typeof node.value !== 'string'
        ) {
          return;
        }
        if (!SUSPECT_STRINGS.has(node.value.trim())) {
          return;
        }
        context.report({node, message: 'Possible JSX Cruft'});
      },
    };
  },
};

Which uncovered hundreds of issues in our large code base with relatively few false positives.

@ljharb
Copy link
Member

ljharb commented Jun 12, 2023

I'm glad you had success with your rule; I'm very skeptical a heuristics-based approach will actually provide few false positives in a general sense (and any false positives are largely unacceptable for a lint rule).

There's legit use cases for having a prose semicolon appear after a jsx element.

@captbaritone
Copy link
Contributor Author

Maybe another way to frame this rule would be, "These syntax fragments are known to be problematic, and therefore we require that they be written explicitly as string literals in order to avoid unintentionally including them in the UI". In that framing the rule becomes more stylistic ("We follow this convention in order to reduce the likelihood of this type of bug"), and can even be auto-fixable if you want. The main requirement, in my estimation, is that when editing a file, a user has the chance to have these issues pointed out to them so that they are not accidentally included.

So, in cases of a legit need for trailing semicolon (for example) you would just write it like so:

return (
  <Wrapper>
    <h1>Hello!</h1>{";"}
  </Wrapper>
);

Which clarifies to the reader, and the linter, that this semicolon is intentional.

@ljharb
Copy link
Member

ljharb commented Jun 12, 2023

What other examples besides a semicolon would you have in mind?

@captbaritone
Copy link
Contributor Author

From the implementation I provided above, you can see that we're currently linting for ;, }, )}, and );.

@captbaritone
Copy link
Contributor Author

Anything I can do to help drive toward an outcome here? I believe this is could be a very impactful rule for the ecosystem.

@ljharb
Copy link
Member

ljharb commented Jun 24, 2023

I’m willing to review test cases, if you’re willing to try some - docs and implementation are optional :-) i don’t want you to do unnecessary work if I’m not convinced, but it’s easiest to understand for me to review actual code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants