feat(@lexical/eslint-plugin): new package with eslint rules for lexical

[etrepum]

@lexical/eslint-plugin - an eslint plugin that helps with the rules of lexical ($functions) and provides an autofixer

Documentation preview: https://lexical-qke2cx8a9-fbopensource.vercel.app/docs/packages/lexical-eslint-plugin

Currently defines one rule: @lexical/rules-of-lexical

There's documentation per #5869 for adding a new package to npm already in the maintainers guide: https://lexical-qke2cx8a9-fbopensource.vercel.app/docs/maintainers-guide#creating-a-new-package

The thing about this package that's atypical relative to how most new packages would be created is that it's entirely written in cjs (not TypeScript or using modules), so that it can be used in the monorepo with our version of eslint without compilation. There are plenty of jsdoc comments for type checking though.

The design is basically that it will look for $function call expressions and see if they were made in an allowed context (e.g. from another $function). If they are not, then it will suggest a rename of fn to $fn.

Some complex situations that rules-of-lexical handles:

Avoid shadowing an existing variable

BEFORE

  const createKeywordNode = useCallback((textNode: TextNode): KeywordNode => {
    return $createKeywordNode(textNode.getTextContent());
  }, []);

AFTER

  const $createKeywordNode_ = useCallback((textNode: TextNode): KeywordNode => {
    return $createKeywordNode(textNode.getTextContent());
  }, []);

Renaming an export

BEFORE

/**
 * Traverses up the tree and returns the first ListItemNode found.
 * @param node - Node to start the search.
 * @returns The first ListItemNode found, or null if none exist.
 */
export function findNearestListItemNode(
  node: LexicalNode,
): ListItemNode | null {
  const matchingParent = $findMatchingParent(node, (parent) =>
    $isListItemNode(parent),
  );
  return matchingParent as ListItemNode | null;
}

AFTER

/**
 * Traverses up the tree and returns the first ListItemNode found.
 * @param node - Node to start the search.
 * @returns The first ListItemNode found, or null if none exist.
 */
export function $findNearestListItemNode(
  node: LexicalNode,
): ListItemNode | null {
  const matchingParent = $findMatchingParent(node, (parent) =>
    $isListItemNode(parent),
  );
  return matchingParent as ListItemNode | null;
}
/** @deprecated renamed to $findNearestListItemNode by @lexical/eslint-plugin rules-of-lexical */
export const findNearestListItemNode = $findNearestListItemNode;

I've tested the output artifact (the npm pack that our integration tests build) with a project outside of the monorepo in this branch: https://github.com/etrepum/lexical-esm-nextjs/tree/lexical-eslint-plugin

I stripped this down to just the new code, it does not include fixes to any existing code.

I believe you should be able to try this out outside of the monorepo before it is released by installing it this way:

npm i --save-dev https://gitpkg.now.sh/etrepum/lexical/packages/lexical-eslint-plugin?lexical-eslint-plugin

To see a concrete example of what this plugin would do in the monorepo, you can check out the last commit in #5979 095502e

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 4, 2024 3:02pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 4, 2024 3:02pm

[Sahejkm]

• This looks great. Thanks for working on this!
• i saw we have some items in our roadmap to add lint rules related to examples like: "prevent mistakes - append inline on RootNode or $createTextNode('\n')" , we could potentially extend this.
• also once this lands, will plan on the script to sync the lint rules to Meta as well

[Sahejkm]

just a question, is this supposed to be array as well like other ?

isSafeDollarFunction: ['$createRootNode'],

[etrepum]

Either works, I wrote it this way mostly to demonstrate that you do not need to wrap it with an array. Basically a convenience for OSS users who may just have one pattern or string to configure.

Here are some of the relevant types, I pasted them here together:

/**
 * @typedef {(name: string, node: Identifier) => boolean} NameIdentifierMatcher
 * @typedef {NameIdentifierMatcher | string | RegExp | undefined} ToMatcher
 * @typedef {Partial<BaseMatchers<ToMatcher | ToMatcher[]>>} RulesOfLexicalOptions
 */
/**
 * @template T
 * @typedef {Object} BaseMatchers<T>
 * @property {T} isDollarFunction Catch all identifiers that begin with '$' or 'INTERNAL_$' followed by a lowercase Latin character or underscore
 * @property {T} isIgnoredFunction These functions may call any $functions even though they do not have the isDollarFunction naming convention
 * @property {T} isLexicalProvider Certain calls through the editor or editorState allow for implicit access to call $functions: read, registerCommand, registerNodeTransform, update.
 * @property {T} isSafeDollarFunction It's usually safe to call $isNode functions, so any '$is' or 'INTERNAL_$is' function may be called in any context.
 */

[Sahejkm]

• right now we have isIgnoredFunction to bypass lint at project level if i understand correctly ?
• Should this isIgnoredFunction be at a rule level as well, apart from having it to ignore all rules for the project

[etrepum]

All configuration here only applies to the @lexical/rules-of-lexical rule. In the example above you can see how it is scoped

{
  "plugins": [
    // ...
    "@lexical"
  ],
  "rules": {
    // ...
    "@lexical/rules-of-lexical": [
      "error",
      {
        "isDollarFunction": ["^\\$[a-z_]"],
        "isIgnoredFunction": [],
        "isLexicalProvider": [
          "parseEditorState",
          "read",
          "registerCommand",
          "registerNodeTransform",
          "update"
        ],
        "isSafeDollarFunction": ["^\\$is"]
      }
    ]
  }
}

I suppose it is somewhat confusing because the name "rules-of-lexical" is plural, so it sounds like it maybe applies to more than one eslint rule, but I just took the naming convention from "react-hooks/rules-of-hooks" since it serves a very similar purpose.

[etrepum]

@Sahejkm it would be great to have a bit more context around what other rules would be useful, and in particular what the suggestions should be. $createTextNode('\n') is a great example of something that could use a little more help than just a lint rule. We have a little bit of documentation about LineBreakNode but the functionality to convert text to an Array of nodes isn't really exposed as far as I can tell, it's hidden inside Selection.insertRawText and the clipboard code. Maybe we should extract that to a utility function which can be suggested in the docs and the lint rule (and maybe even trigger a warning in __DEV__ when $createLineBreakNode is called with embedded newlines or tabs to catch cases that the lint wouldn't see)

[Sahejkm]

Maybe we should extract that to a utility function which can be suggested in the docs and the lint rule (and maybe even trigger a warning in DEV when $createLineBreakNode is called with embedded newlines or tabs to catch cases that the lint wouldn't see)

Yep this looks like a great idea. i quoted the extra lint rule examples from the "Lexical OSS Roadmap 2024" items. sorry for missing the details around it, those could be separate PRs /enhancements.

[Sahejkm]

LGTM

[etrepum]

Latest commit just merges with master to bring in #6019 so the tests pass

[etrepum]

@Sahejkm is this "Lexical OSS Roadmap 2024" published somewhere that I would be able to read it?

[potatowagon]

@Sahejkm is this "Lexical OSS Roadmap 2024" published somewhere that I would be able to read it?

https://docs.google.com/document/d/1Oyv9akr-eF-Zx7qR9vPai3P0bQrU9iJdXg5lbH1aifg/edit#heading=h.dois377swvec

@etrepum

[potatowagon]

great work! im looking forward to adding a lint rule, maybe such as "prefering plain for loop over forEach/map or any lambda function for performance reasons"

to do so would i be adding a lint-rule-name.js to rules folder, and add other stuff using this PR as an example?

if there are any docs i should be aware of do point me to them too!

[etrepum]

This plugin in particular is for rules that we want to offer to OSS users and I think that they should be focused on topics that relate to Lexical usage, not just our preferred code style for the monorepo.

We do have another plugin at ./eslint-plugin which can be used to create custom rules that only apply to the Lexical monorepo.

Those use cases sound like things that could probably be found in an existing eslint plugin somewhere, although I think that it would be tricky to automatically identify situations where we really care about using loops vs. higher-order functions. I would personally find that rule pretty annoying when working on build scripts or tests for example, but if it only applied to dollar functions and LexicalNode methods maybe it would make sense to include it here somehow!

As far as eslint plugin development goes, the resources that I found most useful were:

• https://github.com/facebook/react/tree/main/packages/eslint-plugin-react-hooks (I probably read through a few other plugin sources along the way)
• https://eslint.org/docs/latest/extend/plugins
• https://astexplorer.net/
• Reading through most of the relevant types and some of the source code to most of the things involved in the process.

I was already somewhat familiar from how all of this works from writing codemods and babel/vite/rollup plugins and in general working with ASTs and compilers in other languages. It is a deep topic!

[etrepum]

Latest commit just resolves conflict in package.json and package-json.lock