43081j / eslint-plugin-wc Goto Github PK

I added the plugin, and tested all recommended and best-practice rules. The only rule that didn't behave as described was require-listener-teardown.

My config is pretty barebones, and I reset everything that didn't have to do with this plugin and still no dice.

You could in theory have constructor parameters as long as they are optional:

class Foo extends HTMLElement {
  constructor(a?: string, b?: string) {
    // fine
  }
}

We could update the rule to check for this and allow it

As @bennypowers points out in his tweet, https://twitter.com/PowersBenny/status/1383706876106395648

It can be detrimental to performance to set innerHTML inside the constructor. To accomplish this in a more performant way, you can create a static template and append that to the shadow dom instead.

This would be a fairly easy rule to detect and provide guidance for better perf.

Would validate the element's name against the WC spec name regex.

This would probably fit in a recommended/required ruleset.

This rule was implemented in @polymer/linter https://github.com/Polymer/tools/blob/master/packages/linter/src/html/validate-element-name.ts

That implementation was using the validate-element-name package from npm. https://www.npmjs.com/package/validate-element-name

Looks like the main index is missing a couple of rules here:
https://github.com/43081j/eslint-plugin-wc/blob/master/src/index.ts#L11-L19

In addition, the best-practice config is also missing them as well, which conflicts with what's stated in the README:
https://github.com/43081j/eslint-plugin-wc/blob/master/src/configs/best-practice.ts

Just need to add these missing rules to both places so we can then use them.

window.customElements.define('my-app', MyApp) currently doesn't trigger name validation

We may be merging the custom-elements eslint plugin's extra functionality into this plugin so we can maintain one together.

To do that, we need to:

• Create or copy across any additional rules this plugin doesn't already have
• Update or replace rules we have with their better counterparts (for the ones which are better)

That also means this'll be a breaking change (or many).

Plugin can be found here.

CE rules

• Require customElements.define calls to be after class definitions
• Expose registered classes in the global namespace (window)
• Require that defined elements extend the correct super class (e.g. HTMLElement)
• Require that the filename matches the element name
• Disallow use of the constructor (entirely, in that this isn't the same as wc/no-constructor-params and friends)
• Disallow extending built in elements
• Disallow DOM traversal in attributeChangedCallback
• Disallow DOM traversal in connectedCallback
• Disallow any non-element export in a file containing an exported element
• Disallow on as a prefix for methods
• Require customElements.define calls to be guarded
• Disallow multiple element definitions per file
• Require tag name to match class name (i.e. convert camel to kebab case)
• Require valid tag names

Changes

wc/no-invalid-element-name (DONE)

This one is slightly less robust than the CE rule, so we should add the following functionality:

• options.onlyAlphanum - only allow alpha-numeric names
• options.disallowNamespaces - disallow well known namespaces
• options.suffix - require a suffix on all names
• options.prefix - require a prefix on all names

Currently, we have a options.loose flag which is the equivalent of setting options.disallowNamespaces to false.

For backwards compat, we should probably deprecate loose but alias it to !disallowNamespaces

This backwards compat was discarded since its a breaking change either way here.

New rules

• wc/define-tag-after-class-definition #84
• wc/expose-class-on-global #85
• wc/extends-correct-class #96
• wc/file-name-matches-element #97
• wc/no-customized-built-in-elements #87
• wc/no-dom-traversal-in-attributechangedcallback #88
• wc/no-dom-traversal-in-connectedcallback #89
• wc/no-exports-with-element #94
• wc/no-method-prefixed-with-on #92
• wc/no-unchecked-define #93
• wc/one-element-per-file #90
• wc/tag-name-matches-class #91
• wc/no-constructor #86

Open questions

• Do we want to rename no-unchecked-define to follow eslint's naming, and call it guard-define-call? (e.g. they have guard-for-in, and we have guard-super-call)

cc @keithamus @stramel

The general consensus seems to be that using 'closed' shadowRoots aren't worth the time.

This would probably fit into a "best practices" ruleset.

Resource:
https://developers.google.com/web/fundamentals/web-components/shadowdom#closed
https://developer.mozilla.org/en-US/docs/Web/Web_Components/Using_shadow_DOM#Basic_usage
https://blog.revillweb.com/open-vs-closed-shadow-dom-9f3d7427d1af

no-typos would be a good rule name, following other eslint rule definitions.

It'd be nice to ignore abstract classes by default.
Yes, I know you have to use @typescript-eslint/parser as parser, but please. :P

edit:
Oh, you already depends on typescript-eslint, then it's easy.

The spec expects no attributes to be added during construction. Suggest approach is to delay setting attributes until connectedCallback.

The element must not gain any attributes or children, as this violates the expectations of consumers who use the createElement or createElementNS methods

Resource:
https://stackoverflow.com/questions/43836886/failed-to-construct-customelement-error-when-javascript-file-is-placed-in-head

It should be relatively simple to add a fix for wc/guard-super-call

Take a simple error state such as:

connectedCallback() {
  super.connectedCallback();
  this.setAttribute('bar', 'baz');
}

and change it to this:

connectedCallback() {
  if (super.connectedCallback) {
    super.connectedCallback();
  }
  this.setAttribute('bar', 'baz');
}

I just tried to merge our ce and wc configs and I got this error.

Definition for rule 'wc/define-tag-after-class-definition' was not found

package.json

devDependency: {
'eslint-plugin-wc': '^2.0.0'
}

It's better (more explicit) to guard a super call with a typeof check rather than a "truthy" check. Here's a patch to do that.

diff --git a/node_modules/eslint-plugin-wc/lib/rules/guard-super-call.js b/node_modules/eslint-plugin-wc/lib/rules/guard-super-call.js
index 873ec0d..ad659e4
--- a/node_modules/eslint-plugin-wc/lib/rules/guard-super-call.js
+++ b/node_modules/eslint-plugin-wc/lib/rules/guard-super-call.js
@@ -65,6 +65,19 @@ const rule = {
                 node.expression.type === 'CallExpression' &&
                 isSuperHook(node.expression.callee, hook));
         }
+        /**
+         * Determines if an if statement is a correct super hook guard
+         * @param {ESTree.IfStatement} node Node to test
+         * @param {string} hook hook to test
+         * @return {boolean}
+         */
+        function isCorrectSuperHookGuard(node, hook) {
+            return node.test.type === 'BinaryExpression' &&
+                node.test.left.operator === 'typeof' &&
+                isSuperHook(node.test.left.argument, hook) &&
+                node.test.right.type === 'Literal' &&
+                node.test.right.value === 'function';
+        }
         /**
          * Determines if a statement is an unguarded super hook
          * @param {ESTree.Statement} node Node to test
@@ -76,7 +89,7 @@ const rule = {
                 errNode = node;
                 return true;
             }
-            else if (node.type === 'IfStatement' && !isSuperHook(node.test, hook)) {
+            else if (node.type === 'IfStatement' && !isCorrectSuperHookGuard(node, hook)) {
                 return isUnguardedSuperHook(node.consequent, hook);
             }
             else if (node.type === 'BlockStatement' &&

It should be relatively easy to produce a fix for wc/no-closed-shadow-root rule on error.

Taking .attachShadow({ mode: 'closed' }) to .attachShadow({ mode: 'open' })

Elements that need to express their state should do so using attributes. The class attribute is generally considered to be owned by the developer using your element, and writing to it yourself may inadvertently stomp on developer classes.

This would probably fit in a best practice ruleset.

Resource:
https://developers.google.com/web/fundamentals/web-components/best-practices#do-not-self-apply-classes

I've been doing a lot of workshops where people learn to use lit-html. One of the most common mistakes is to forget calling super.connectedCallback() when using lit-element.

This would be a good linting rule to add.

This is a mirror issue of 43081j/eslint-plugin-lit#36, opened on request.

Ensure the attachShadow is only called in the constructor. This may only apply to classes that extend HTMLElement.

The constructor is when you have exclusive knowledge of your element. It's a great time to setup implementation details that you don't want other elements messing around with. Doing this work in a later callback, like the connectedCallback, means you will need to guard against situations where your element is detached and then reattached to the document.

Resource:
https://developers.google.com/web/fundamentals/web-components/best-practices#create-your-shadow-root-in-the-constructor

Hey guys! First of all, thank you for such a cool plugin!

Since ESLint 9 is soon to be released, are there any plans to add a Flat config and, equally importantly, improve the d.ts files of the plugin?

eslint.config.js:25:3 - error TS2322: Type 'typeof import("node_modules/eslint-plugin-wc/lib/index")' is not assignable to type 'Plugin'.
  Types of property 'configs' are incompatible.
    Type '{ recommended: { plugins: string[]; parserOptions: { sourceType: string; }; rules: { 'wc/no-constructor-attributes': string; 'wc/no-invalid-element-name': string; 'wc/no-self-class': string; }; }; 'best-practice': { ...; }; }' is not assignable to type 'Record<string, FlatConfig | ConfigData<RulesRecord> | FlatConfig[]>'.
      Property 'recommended' is incompatible with index signature.
        Type '{ plugins: string[]; parserOptions: { sourceType: string; }; rules: { 'wc/no-constructor-attributes': string; 'wc/no-invalid-element-name': string; 'wc/no-self-class': string; }; }' is not assignable to type 'FlatConfig | ConfigData<RulesRecord> | FlatConfig[]'.
          Type '{ plugins: string[]; parserOptions: { sourceType: string; }; rules: { 'wc/no-constructor-attributes': string; 'wc/no-invalid-element-name': string; 'wc/no-self-class': string; }; }' is not assignable to type 'ConfigData<RulesRecord>'.
            The types of 'parserOptions.sourceType' are incompatible between these types.
              Type 'string' is not assignable to type '"script" | "module" | undefined'.

25   wc,
     ~~

eslint.config.js:27:2 - error TS2322: Type '{ 'wc/guard-super-call': ["off"]; 'wc/attach-shadow-constructor': string; 'wc/no-child-traversal-in-attributechangedcallback': string; 'wc/no-child-traversal-in-connectedcallback': string; ... 5 more ...; 'wc/require-listener-teardown': string; }' is not assignable to type 'RulesRecord'.
  Property ''wc/attach-shadow-constructor'' is incompatible with index signature.
    Type 'string' is not assignable to type 'RuleEntry<any[]>'.

27  rules: {
    ~~~~~

  ../../node_modules/@types/eslint/index.d.ts:1116:9
    1116         rules?: RulesRecord;
                 ~~~~~
    The expected type comes from property 'rules' which is declared here on type 'FlatConfig'

Some Element definitions may not include all the hooks that HTMLElement provides, so when extending these Elements, a check should occur before calling.

For instance, say you want to extend MyElement which doesn't implement connectedCallback

class MyElement extends HTMLElement {
  foo = 'bar'
}

extending it and using the connectedCallback like so will result in an error

class MyOtherElement extends MyElement {
  connectedCallback() {
    super.connectedCallback() // ERROR: super.connectedCallback is undefined
    this.setAttribute('bar', 'baz')
  }
}

Adding a check ensures that it doesn't error if it doesn't exist.

class MyOtherElement extends MyElement {
  connectedCallback() {
	if (super.connectedCallback)
      super.connectedCallback() // No Error 😄 
    this.setAttribute('bar', 'baz')
  }
}

Per lit documentation, removeEventListener is not required if this is used (as I presume everything will be garbage collected together).

Ideally wc/require-listener-teardown should be configurable to warn on

• anonymous functions in addEventListener (inside connectedCallback).
• ignore this (only warn on window, or document for example).

There are some high severity issues after running npm audit.

❯ npm audit                                                                                                                    (master|✔)
# npm audit report

normalize-url  4.3.0 - 4.5.0 || 5.0.0 - 5.3.0 || 6.0.0
Severity: high
Regular Expression Denial of Service - https://npmjs.com/advisories/1755
fix available via `npm audit fix`
node_modules/normalize-url

trim-newlines  <3.0.1 || =4.0.0
Severity: high
Regular Expression Denial of Service - https://npmjs.com/advisories/1753
fix available via `npm audit fix --force`
Will install [email protected], which is a breaking change
node_modules/trim-newlines
  meow  3.4.0 - 5.0.0
  Depends on vulnerable versions of trim-newlines
  node_modules/meow
    validate-element-name  2.0.0 - 2.1.1
    Depends on vulnerable versions of meow
    node_modules/validate-element-name

4 high severity vulnerabilities

I can try to create a PR to update some dependencies. The main one I detect is validate-element-name is now in version 3

I have seen quite a bit recently a pattern of a static getter or property for the component name. We could fairly easily add an opt-in rule for this.

Something like require-static-name with a default config { propertyName: 'is' } that could be changed to something like { propertyName: 'tag' } instead?

Currently wc/no-invalid-element-name disallows the usage of polymer-, ng-, x- tags, where there is nothing in the spec against this.

There should be a configuration that allows teams to indicate their preferred namespace to have consistency in a project, and allow namespaces like x-

Global attributes are those that are present on all HTML elements. Some examples include tabindex and role. A custom element may wish to set its initial tabindex to 0 so it will be keyboard focusable. But you should always check first to see if the developer using your element has set this to another value. If, for example, they've set tabindex to -1, it's a signal that they don't wish for the element to be interactive.

Ensure that there is a check for the attribute before altering (set/remove) the attribute in the connectedCallback.

if (!this.hasAttribute('foo')) this.setAttribute('foo', 'bar')

Resource:
https://developers.google.com/web/fundamentals/web-components/best-practices#do-not-override-author-set-global-attributes
https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes

Currently an npm pack or publish will produce unexpected results.

We should add a files entry to our package.json to define what should be packed:

• lib/
• docs/
• README.md

at least.

Using this.shadowRoot.append() in a WC constructor leads to a wc/no-constructor-attributes error.

class Demo extends HTMLElement {
  constructor() {
    super();
    this.attachShadow({ mode: "open" });

    // ERROR: Attributes must not be interacted with in the constructor as the element may not be ready yet.
    // eslintwc/no-constructor-attributes
    this.shadowRoot.append(
      p(button("previous", "<"), button("next", ">"))
    );
  }
}

Yet, the standard only restricts manipulations of the light DOM, and manipulating the Shadow DOM doesn't lead to any runtime error.

IMO, this is a false positive.

eslintrc:
wc/file-name-matches-element': [ 2, { transform: 'none' } ]

/components/BaseBucketHolder/BaseBucketHolder.tsx
14. export default abstract class BaseBucketHolder extends HTMLElement {
(no customElements.define because it's an abstract class)
14:31 error File name should be "" but was "BaseBucketHolder" wc/file-name-matches-element

/components/Bucket/Bucket.tsx
16. export default class Bucket extends HTMLElement {
...
110: customElements.define('sni-bucket', Bucket)
16:22 error File name should be "" but was "Bucket" wc/file-name-matches-element

And we got many-many more.

It worked well when we used ce-plugin.

edit:
(as I see you don't have tests for .ts(x) filenames, nor abstract classes which extends from HTMLElement without registering in customElements)