Add ESLint

[CreativeTechGuy]

What issue are you having?

As a newcomer to the project, it's hard to figure out what the correct coding style is and best practices. When looking around the files, I see different approaches for the same thing. (eg: a few functions have explicit return types, others don't. Sometimes named functions use arrow-syntax and others function keyword. Type imports are sometimes imported with import type and other times not.) As a result, it's confusing when jumping into the code base as it's unclear what standards to follow.

Another big area of difficulty is the usage of loose TypeScript types such as any throughout the codebase. If you don't realize a variable you are working with is any you may write code that passes validations but fails at runtime.

It would also be nice to have additional warnings regarding some of the specific tools used (lit, web components, chai) to be able to warn me against easy errors like forgetting to remove my event listeners or writing invalid HTML in template strings.

Describe the solution you'd like

I'd like the library to setup ESLint to enforce consistent code (both style and functionality).

Describe alternatives you've considered

I could create PRs and wait to be told by the maintainers that my code isn't correct, or be responsible for bugs due to errors I've introduced without realizing. Honestly though, it'd be easier to not contribute to the library as I feel like I'm too likely to screw something up if I make a change.

ESLint specifics

I'd like to add ESLint to this package and use this issue thread to discuss any controversial rule decisions along the way. Here's a few of the larger (possibly controversial) decisions to decide upfront if we are to do this.

• Should we add support for absolute imports instead of relative parent imports. This would involve updating the build/TS to support something like ~/ to reference src/ and then add the ESLint rules to prohibit any ../* imports. The benefit would be much clearer import paths instead of traversing up the tree and easier refactoring as the paths are not parent-relative. A downside is the added build complexity (which would be solved by a bundler like Rollup/Webpack, but I'm not sure about ESBuild.
• Should we enforce explicit boundary types and function return types. This does two things, makes the function signature obvious when looking at it as it's explicitly defined. Also helps catch errors inside of a function if somehow it returns a type that you didn't expect. This catches errors in functions much closer to the source where as implicit return types will just automatically update and the error may be found elsewhere in the code.
• We can define a naming convention to use for various types of variables/properties/functions/types/etc. Any restrictions that should be put in place for naming anything? See this doc for the different selectors that can be used.
• Should we enforce explicit member access modifiers? So every class property/method must include private, public, or protected.
• Should we prohibit the use of any (to be replaced with unknown or a better type when possible)? Similarly, prohibit other types which aren't as strict or accurate as they might seem (like: Function, {}, Object, etc)
• Should we prohibit implicit type conversions? So a conditional expression must evaluate to only true/false, not any truthy/falsy value. And you must use parseInt, .toString(), etc to convert between types rather than implicitly with operators like +.
• What about import order? While the order itself may not matter, it can be helpful for automatically reformatting and reordering imports with some rules. I'd suggest the order of "builtin" then "external" then "internal" with no newlines in between and alphabetical.
• And a miscellaneous style thing: I think there should be both error and warn levels for lint rules, but both should be enforced as if they were errors. The benefit to this is that with the IDE integration, a yellow underline (for warn) indicates that it is non-critical and is likely a style issue and doesn't need to be fixed immediately, but a red underline (for error) indicates that it is likely important to fix immediately as it may cause bugs or confusion and it'd be better to fix it now. Since both are enforced on commit/CI, neither will exist in the codebase, but during development these color codings will help the developer prioritize the squiggly lines as they go.

And all of these decisions are not permanent. If a rule becomes a pain it can be changed or removed. If there are cases where it doesn't apply but is generally useful, it can be disabled with a comment in the code. As long as these changes provide benefit and security overall, they are going to pay for themselves!

[claviska]

When looking around the files, I see different approaches for the same thing. (eg: a few functions have explicit return types, others don't. Sometimes named functions use arrow-syntax and others function keyword. Type imports are sometimes imported with import type and other times not.) As a result, it's confusing when jumping into the code base as it's unclear what standards to follow.

I agree this could be better, but a balance is necessary to encourage both consistency and creativity. There's nothing worse than getting into flow with an idea and then fighting with types or linters that are so strict that they cause you to stumble. 😅

For example, I usually type things as I go, but sometimes (especially with third party types I'm less familiar with) I plow through it to make sure the concept works before spending too much time on types. I'd hate to lose this ability.

Should we add support for absolute imports instead of relative parent imports.

I'm OK with doing this as long as it's done cleanly through esbuild. Per this and this, it looks like it's possible with path aliasing in tsconfig. I don't want to introduce another build tool at this time.

Should we enforce explicit boundary types and function return types.

Let's hold off on this one for now.

We can define a naming convention to use for various types of variables/properties/functions/types/etc.

I'd be interested to see how well the codebase fairs against this rule, so let's enable it and see.

Should we enforce explicit member access modifiers?

I've been sticking to private fields only because it got super verbose along with decorators, types, JSDoc comments, etc. I'm probably missing a few private keywords, but it feels way more readable without redundant public keywords everywhere. (This is mostly preference.)

Should we prohibit the use of any (to be replaced with unknown or a better type when possible)?

I'm OK with forbidding this. There aren't that many in the project, but there are some that need to be updated.

Should we prohibit implicit type conversions?

Sure, let's give it a go.

What about import order?

Totally fine. Preferably it will update the order on save (can Prettier do this?) so you don't have to do it manually.

I think there should be both error and warn levels for lint rules, but both should be enforced as if they were errors.

Yeah, we don't want them being committed. At the same time, when I'm developing locally, if I knowingly "break" types to test something out, I still want it to rebuild. This is how it currently works with esbuild (because it discards types) so those things are currently caught by TypeScript.

Basically I'm fine showing the errors via ESLint and stopping the build, but it shouldn't hinder local dev as temporary breakages are often intentional.

And all of these decisions are not permanent. If a rule becomes a pain it can be changed or removed.

For sure. Thanks again for taking the time to sort through this and offer your thoughts!

[CreativeTechGuy]

For example, I usually type things as I go, but sometimes (especially with third party types I'm less familiar with) I plow through it to make sure the concept works before spending too much time on types. I'd hate to lose this ability.

Yup totally! I'll make sure that this doesn't impact development. This should only be enforced on commit and on PR. During development you can do whatever you want as long as it complies by the time it's done. :)

I'm OK with doing [absolute imports] as long as it's done cleanly through esbuild. I don't want to introduce another build tool at this time.

I'll try this out and see if it's easy enough to add without impacting anything. This will be TBD on implementation difficulties.

I'd be interested to see how well the codebase fairs against [the naming convention] rule, so let's enable it and see.

The naming convention rule is super customizable. Do you have any rules which we should try to enforce or should I just try to reverse engineer what is currently there and standardize on whatever is the most common currently?

Totally fine [to enforce import order]. Preferably it will update the order on save (can Prettier do this?) so you don't have to do it manually.

ESLint has a --fix flag which will be used on commit to auto fix where possible and I believe you can setup your IDE to auto-fix on save or you can use the ESLint formatter in the right-click menu to do it on-demand. So yeah you should never need to manually reorder imports, it'll be auto-fixable!

---

With your answers to the above, I should have enough to get started. Thanks for your support!

[claviska]

Do you have any rules which we should try to enforce or should I just try to reverse engineer what is currently there and standardize on whatever is the most common currently?

I was thinking standardize what's there/common.

Let me know if you need anything else!

[CreativeTechGuy]

Status update:

I'm still chugging away. I think there were about 2,000 errors/warnings when I started, now we are down to about 800. So definite progress being made. This is going to be a massive PR since it touches basically every file.

One weird thing that I encountered for which I'm proposing a functionality change (minor) is that the @query decorator only grabs the elements after the first render. This means that the elements will be null for any methods which are decorated in @watch since those update immediately. I could update the types of all @query decorated properties to include | null but that then would require checks everywhere it is used, not just in places where it's used on the first render. Technically that is true, but it isn't super useful in most cases. I was able to update the @watch decorator to run the method async if the watch is triggered on the first update. This seems to solve the problem and can remove a lot of null checks that were present around the code. I don't know if this will cause other issues that I'm unaware of, but it seems to be working well so far.

Here's the snippet of relevant code to help explain the change to @watch I'm proposing since I know the paragraph above is gnarly. haha

if (oldValue !== newValue) {
  if (!resolvedOptions.waitUntilFirstUpdate || this.hasUpdated) {
    // Make the first update async to prevent the elements from @query being null
    if (!this.hasUpdated && !resolvedOptions.waitUntilFirstUpdate) {
      setTimeout(() => {
        (this[decoratedFnName] as unknown as UpdateHandler)(oldValue, newValue);
      });
    } else {
      (this[decoratedFnName] as unknown as UpdateHandler)(oldValue, newValue);
    }
  }
}

[claviska]

Just a heads up...I noticed form controls were rendering strangely (notice the extra margin on top):

I traced it to this code in src/internal/form-control.ts:

const hasLabel = typeof props.label !== 'undefined' ? true : props.hasLabelSlot === true;
const hasHelpText = typeof props.helpText !== 'undefined' ? true : props.hasHelpTextSlot === true;

The original code looked like this:

const hasLabel = props.label ? true : !!props.hasLabelSlot;
const hasHelpText = props.helpText ? true : !!props.hasHelpTextSlot;

They aren't equivalent. While the latter violates a couple of the new lint rules, it works as expected whereas the new condition fails due to the stricter undefined check. (In the case of form controls, props.label is defined as a non-empty string and will never be undefined.)

This was one of the concerns I had with enabling this rule, verbosity being the other. I'll dive deeper into testing to make sure we don't introduce any regressions due to similar logic updates.

[CreativeTechGuy]

Oh no! I feel awful. I assure you that the motivation for these changes was to make it better and safer, not worse. I apologize. I'm sorry you've had to revert a good portion of it. Let me know if there's anything you need from me.

[claviska]

No worries. It was only two or three rules and a handful of associated lines. Everything else is fantastic!

I didn’t write this to chastise your efforts. I figured it would be worth mentioning to make future refactors (here or anywhere you contribute) more smooth. And also in case you were wondering why the affected lines got reverted.

You’ve nailed a few really solid PRs and I’m very thankful for your contributions!