Skip to content
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

Add "deno lint" subcommand #1880

Open
ry opened this issue Mar 5, 2019 · 20 comments
Open

Add "deno lint" subcommand #1880

ry opened this issue Mar 5, 2019 · 20 comments
Labels

Comments

@ry
Copy link
Collaborator

@ry ry commented Mar 5, 2019

For linting files.

Probably should be implemented similar to how we've done --fmt ... that is get eslint into deno_std, and then load from URL in core.

@zekth

This comment has been minimized.

Copy link
Contributor

@zekth zekth commented Mar 6, 2019

eslint and those plugins:

  • @typescript-eslint/eslint-plugin
  • @typescript-eslint/parser
  • eslint-config-prettier

So in term of updates, Deno will update on major releases of Typescript / eslint? To stay as tied as the LTS version of those dependencies.

@kitsonk

This comment has been minimized.

Copy link
Contributor

@kitsonk kitsonk commented Mar 6, 2019

Also, eslint needs access to TypeScript to perform its duties, currently the Deno runtime does not give access to TypeScript, it only exists in the compiler.

@tunnckoCore

This comment has been minimized.

Copy link

@tunnckoCore tunnckoCore commented Sep 9, 2019

Agree with @zekth, plus probably eslint-plugin-prettier.

Also believe that if/when this is done, we can allow deno fmt to actually also use the eslint (and also the autofix).

I almost always have autofix enabled, both when using through terminal and in vscode. So it picks the eslint config AND the prettier config to automatically fix and format everything. Great consistency.

It doesn't even matter what I write and how I write it. I'm always a 1ms away from Ctrl+S or on pre-commit. I don't need to care or even know what tha heck I'm doing, the macine/tool/system is smart enough to correct me, help me and guide me if I'm wrong.

@tunnckoCore

This comment has been minimized.

Copy link

@tunnckoCore tunnckoCore commented Nov 21, 2019

The deno fmt can be just deno lint with just try/catch and always exit with 0 code.

side thoughts:
I have pretty robust config using Airbnb, typescript-eslint, Prettier (the plugin & config), React, plugin-import, plugin-unicorn, plugin-promise, plugin-node, config XO, XO-typescript. With their recommended presets and few customizations (with described reasons as comments) for the modern day and age, and more cleaner & sane code. And I have to tell you that I'm using this to that extent for around 1-2 years (Airbnb user for 3+ years), and it's insanely well.

I'm going to market it and promote it more, probably in the beginning of next year, because I really think it at least worth reviews and discussions.

All this should be build into "the language"/runtime, then easily write a thin wrapper arond the main CLI binary and voila you have editor integrations. From there, you have unlimited powered and amazing DX.

@kt3k

This comment has been minimized.

Copy link
Contributor

@kt3k kt3k commented Nov 22, 2019

Note about implementation:

https://github.com/mysticatea/typescript-eslint-demo

ESLint doesn't support browser officially, but one of the eslint core contributers, @mysticatea (Toru Nagashima), maintains browser version of eslint engine at https://github.com/mysticatea/eslint4b . The above demo uses this module as eslint engine. Probably we can reuse this module in Deno and are able to perform eslint checks.

@tunnckoCore

This comment has been minimized.

Copy link

@tunnckoCore tunnckoCore commented Nov 30, 2019

I don't think we need "browser support"-ed eslint. Deno is not run in browser, but in server-side as cli.

The only problem might be the size of Deno, which may increase drastically. For some time I'm thinking to bundle ESLint with @zeit/ncc to see what will be the size.

@bartlomieju bartlomieju changed the title deno --lint Add "deno lint" subcommand Dec 28, 2019
@bartlomieju bartlomieju added the feat label Dec 28, 2019
@brandonkal

This comment has been minimized.

Copy link
Contributor

@brandonkal brandonkal commented Feb 13, 2020

This could be useful without eslint to replace tsc --noEmit.

@trevordmiller

This comment has been minimized.

Copy link

@trevordmiller trevordmiller commented Mar 10, 2020

I would suggest having the linting setup include preventing errors out of the box; perhaps extending eslint:recommended (which is common problems only), something like:

{
    "parserOptions": {
      "ecmaVersion": X
    },
    "extends": [
      "eslint:recommended"
    ]
}
@nayeemrmn

This comment has been minimized.

Copy link
Contributor

@nayeemrmn nayeemrmn commented Mar 12, 2020

Is there any update on the plan for deno lint, considering std subcommands aren't a thing anymore?

@bartlomieju

This comment has been minimized.

Copy link
Contributor

@bartlomieju bartlomieju commented Mar 19, 2020

Is there any update on the plan for deno lint, considering std subcommands aren't a thing anymore?

@nayeemrmn most likely it will be implemented in Rust using swc.

@tunnckoCore

This comment has been minimized.

Copy link

@tunnckoCore tunnckoCore commented Mar 19, 2020

@bartlomieju how? swc isn't for linting.

@axetroy

This comment has been minimized.

Copy link
Contributor

@axetroy axetroy commented Mar 19, 2020

Here I give my personal opinion:

Do not allow the user to customize linter's rulers

The community has spent too much time arguing which style is better

No need to waste more time on things like this

The user has only one choice, with use or not use

keeping the style consistent is important

Otherwise, in the future, tools like eslint/tslint/denolint will be born

and users are confused with "Which one should I choose?"

@kitsonk

This comment has been minimized.

Copy link
Contributor

@kitsonk kitsonk commented Mar 19, 2020

@tunnckoCore swc wasn't for formatting, and along came dprint.

@tunnckoCore

This comment has been minimized.

Copy link

@tunnckoCore tunnckoCore commented Mar 20, 2020

@kitsonk sorry what? I'm not saying that. I understood that @bartlomieju's comment implies that swc will be used for the deno lint, which isn't making sense as far as I know.

@bartlomieju

This comment has been minimized.

Copy link
Contributor

@bartlomieju bartlomieju commented Mar 20, 2020

@tunnckoCore I will write linter myself based on swc

@brandonkal

This comment has been minimized.

Copy link
Contributor

@brandonkal brandonkal commented Mar 20, 2020

@axetroy If you say there are too many options, why build out a deno lint at all? That's only adding more options. We already have a linting tool for linting JavaScript and TypeScript and it is called eslint. I don't see a value in duplicating all that work again.

Unless a developer only writes backend code, they will still need to touch eslint.

If a parallel lint tool must be built (are there really not better ways to improve deno?) then it MUST be able to consume existing eslint rules. Otherwise people will waste their time reinventing the wheel, writing duplicate rules for deno and frontend code rather than application code.

Instead deno lint would be more useful as something like golangci-lint where it aggregates and calls other already written lint rules and linters. It provides a "default lint config" (via url import) with sane default but still extendable.

@tunnckoCore

This comment has been minimized.

Copy link

@tunnckoCore tunnckoCore commented Mar 20, 2020

@bartlomieju, wooow. Interesting, please ping somewhere someday. :) I'm really digging into linting with node/deno/rust wrapper for eslint tho, because I already fixed the config resolving (partially implementing rfc#9) slowness from which comes the huge bottleneck in perf. The thing is that linter.verify(AndFix) is noticeably slow too.

But I'm also thinking that thebest way would be to implement it like the TypeScript stuff is done. Instead of compiling it would pass the text to the linter api (my wrapper already exposes pretty good amount of APIs and its internals).

@bartlomieju

This comment has been minimized.

Copy link
Contributor

@bartlomieju bartlomieju commented Mar 21, 2020

@bartlomieju, wooow. Interesting, please ping somewhere someday. :) I'm really digging into linting with node/deno/rust wrapper for eslint tho, because I already fixed the config resolving (partially implementing rfc#9) slowness from which comes the huge bottleneck in perf. The thing is that linter.verify(AndFix) is noticeably slow too.

Sure, I'll let you know.

But I'm also thinking that thebest way would be to implement it like the TypeScript stuff is done. Instead of compiling it would pass the text to the linter api (my wrapper already exposes pretty good amount of APIs and its internals).

Can you elaborate? I have JSON AST that is similar to Babel's, can you feed it to the API you're talking about?

@tunnckoCore

This comment has been minimized.

Copy link

@tunnckoCore tunnckoCore commented Mar 22, 2020

to implement it like the TypeScript stuff is done

With that I meant, that the lint part can be done like the typescript part here in deno, like instead of using typescript use eslint there.

can you feed it to the API you're talking about

No. There are no publicly exposed methods in eslint package to work with AST, only the Linter ones which are accepting only text string. But always thought we, the developers, should have access to such.

@bartlomieju

This comment has been minimized.

Copy link
Contributor

@bartlomieju bartlomieju commented Apr 6, 2020

Update: linter is currently WIP and located at: https://github.com/denoland/deno_lint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants
You can’t perform that action at this time.