Skip to content

Set up eslint and remove tslint #1293

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

Merged
merged 9 commits into from
May 27, 2020
Merged

Set up eslint and remove tslint #1293

merged 9 commits into from
May 27, 2020

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented May 26, 2020

During review of #1240 it came up that we are still lacking a proper linter to identify common style issues before these end up in review. In hindsight, part of the problem was that we bet on tslint initially but it then became deprecated half-way, so we ended up in a situation where investing any more time into it seemed like a waste of effort and replacing it was just so much boring work that it has been postponed over and over.

Therefore, this PR removes tslint and sets up eslint once and for all, but also ended up touching a ton of code with some of the changes seeming problematic. The old custom tslint rules are gone for now, but we can of course make new ones eventually.

  • typescript-eslint doesn't like decorators before const in its indent rule, but this was fixable somewhat by putting the decorators on the same line. Not great, but not a blocker either.
  • Quite a few any, object and {} errors turned up (again) during the process, with Record<string,unknown> seeming like the right alternative, but this has shot us in the foot multiple times in the past. Quite likely that it will backfire once again.
  • Something seems broken in the indent rule in general, disliking specific multi line ternary if patterns. I was able to at least suppress the errors by adding a few silly ignores, but I'm still suspecting that I'm doing something wrong since it seems unlikely that nobody using eslint ever noticed this.

@dcodeIO
Copy link
Member Author

dcodeIO commented May 26, 2020

@jtenner Could you check that the Record<string,unknown> changes here don't cause the issue on your side that we fixed earlier again?

@dcodeIO dcodeIO marked this pull request as ready for review May 26, 2020 11:05
@dcodeIO
Copy link
Member Author

dcodeIO commented May 26, 2020

Regarding custom rules, the next step should probably be to create an AssemblyScript ESLint plugin (i.e. used as plugin:@assemblyscript/eslint). I don't consider this a high priority for this PR, though, because the previous custom rules rarely helped us on the compiler side, yet I understand that such a plugin would be super useful for AS users, as there's an ESLint extension for VSCode that could pick it up and highlight problems as one types.

Also, asinit could automatically add the plugin along a preconfigured ESLint to make using it easy, and suggest installing the VSCode extension.

@@ -100,22 +100,25 @@ export interface ASUtil {
}

/** Asynchronously instantiates an AssemblyScript module from anything that can be instantiated. */
export declare function instantiate<T extends {}>(
export declare function instantiate<T extends Record<string,unknown>>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the correct type should be [key: string]: Record<string, any> and not Record<string, unknown>.

Is there a reason why we can't use any here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm mostly interested in is whether it breaks your code the way it is now or not. From your comment, I assume it does?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Steps I used to test this:

  1. Copied the file to a local installation of AS
  2. imported the loader
  3. ran tsc and asserted there were no compile time errors using latest stable typescript

My software did not break.

I also suppose the answer to my question is that you prefer not to use any if possible. This is fair. Thanks for this work! It's going to help a lot of people in the long run.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with any is mostly that the linter will complain, and we'd have to add an // eslint-disable directive where used, which I'm trying to avoid :)

Copy link
Contributor

@jtenner jtenner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking this for approval because it doesn't break the loader types as requested by @dcodeIO.

I should probably also check this pull request to help out with the es-lint rules too, but I've been incredibly busy.

@dcodeIO
Copy link
Member Author

dcodeIO commented May 27, 2020

Merging to aid PRs depending on it. It's likely that we'll find missing or not yet ideal rules with time, so let's keep an eye open for these and add them when we spot them.

@dcodeIO dcodeIO merged commit 861a97b into master May 27, 2020
@dcodeIO dcodeIO deleted the lint branch June 11, 2020 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants