stencil-community / stencil-eslint

ESLint rules specific to Stencil JS projects
MIT License
61 stars 34 forks source link

bug: props unrelated to HTMLElement are being flagged by `reserved-member-names` rule #100

Closed jcfranco closed 7 months ago

jcfranco commented 10 months ago

Prerequisites

Stencil ESLint Version

0.7.1

Current Behavior

https://github.com/stencil-community/stencil-eslint/pull/67/files introduced properties unrelated to HTMLElement that are being flagged.

For example, a color prop will produce a warning/error despite it not being defined in HTMLElement:

126:28 error The @Prop name "color conflicts with a key in the HTMLElement prototype. Please choose a different name @stencil-community/reserved-member-names

Expected Behavior

Only attributes/properties on HTMLElement.prototype to be flagged by the rule.

Steps to Reproduce

The following should produce the error described above:

import { Component, Prop, h } from '@stencil/core';

@Component({
  tag: 'my-component',
  shadow: true,
})
export class MyComponent {
  @Prop() color: string;

  render() {
    return <div>color is {this.color}</div>;
  }
}

Code Reproduction URL

https://github.com/ionic-team/stencil-component-starter

Additional Information

No response

jcfranco commented 10 months ago

I shamelessly took a shortcut with the repro URL and steps, but please LMK if this is still needed and I'll make the appropriate changes.

raphaelpor commented 10 months ago

Hello @jcfranco, Thanks for reporting that. But I checked again and color is an expected HTML attribute. Specifically used in the <font> element (Ref.: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/font#color).

I understand it is not a common practice, but since we need to complain with the HTML standards, I would recommend keeping it there.

What do you think? :)

jcfranco commented 10 months ago

Thanks for the quick reply!

The doc you referenced is specific to the <font> element or HTMLFontElement, which extends HTMLElement (see https://developer.mozilla.org/en-US/docs/Web/API/HTMLFontElement).

If we test for color in the prototype and instances, you can see the property does not exist in HTMLElement, which is the base class for Stencil/web components.

Worth noting that color is not a global attribute either, so it shouldn't be flagged by the rule as far as I understand it's purpose.

raphaelpor commented 10 months ago

You are right, @jcfranco.

Thanks for the patience to explain it. I will create a PR removing that lock for color, soon. 🙏

jcfranco commented 10 months ago

@raphaelpor Hope you don't mind, but I took a stab at this. 😅

jcfranco commented 8 months ago

@raphaelpor Friendly ping. 😁

raphaelpor commented 8 months ago

@raphaelpor Friendly ping. 😁

Thanks for reminding me @jcfranco. I will make sure to have a look at that, this week. 🙏

James-Wilkinson-git commented 8 months ago

If you use svelte any HTML elements attribute is reserved and it will yell at you when inserting a web component with any of these https://www.w3schools.com/tags/ref_attributes.asp

jcfranco commented 8 months ago

@James-Wilkinson-git I think that's outside of the scope of this rule. Custom elements extend HTMLElement, so reserved names should be in the context of that class. Frameworks warning against attributes/properties from elements extending HTMLElement is a separate issue unrelated to Stencil.

James-Wilkinson-git commented 8 months ago

It’s a best practice to not use any reserved html element not just global ones, as your web component should work everywhere, if you use a reserved attribute for another element this breaks and there should be a rule to guard against it.

This change would greatly reduce the guards for reserved attributes when we actually need to extend the guard to protect against more reserved names so that components work everywhere.

In your example colour should through a warning as there is an html element that uses it. If you inserted this web component into Svelte you wouldn’t be able to bind to your colour attribute because it’s a reserved name. The list as is, is missing several other reserved names such as open and checked

James-Wilkinson-git commented 8 months ago

I think a better AirTag that is more backwards compatible is to create two rules. One for global attributes and one for all attributes. All attributes is needed for anyone who’s consumer acts like svelte and manes assumptions on how to handle each HTML attribute for binding.