New: Adds new info flag

[jamesgeorge007]

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[x] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)
eslint --info / eslint -i

 Environment Information:-

  System:
    OS: Linux 4.18 Ubuntu 19.04 (Disco Dingo)
    CPU: (4) x64 Intel(R) Core(TM) i5-7200U CPU @ 2.50GHz
  Binaries:
    Node: 8.11.4 - /usr/bin/node
    Yarn: 1.17.0-0 - /usr/local/bin/yarn
    npm: 6.10.2 - /usr/local/bin/npm
  Browsers:
    Chrome: 76.0.3809.87
    Firefox: 68.0.1
  npmGlobalPackages:
    eslint: 6.1.0

Is there anything you'd like reviewers to focus on?
Fixes #11958

[jsf-clabot]

All committers have signed the CLA.

[g-plane]

Thanks for contributing!

Can you add tests for your changes?

Also, I think there's no need to print browsers information.

[kaicataldo]

Thanks for the PR!

[ilyavolodin]

Can we see if we can somehow figure out which version of ESLint is being executed? Global or local? Also would be awesome if we could pull and print eslint plugins installed as well, not sure it's possible though.

[kaicataldo]

To clarify, it seems like we have a few options:

1. Make cli.execute() consistently return a promise (which will force consumers to update to handle the asynchronous method correctly)
2. Figure out a way to do this synchronously
3. Decide we're okay with cli.execute() having side effects with a dangling promise

Thoughts from the rest of the team?

[platinumazure]

@ilyavolodin @kaicataldo

Can we see if we can somehow figure out which version of ESLint is being executed? Global or local? Also would be awesome if we could pull and print eslint plugins installed as well, not sure it's possible though.

The closest I've come to that is printing the ESLint execution location in some of our friendly message templates, which can at least help us recognize a global ESLint.

To clarify, it seems like we have a few options:

I think we should try for synchronous first, but if that's not possible, I think it would be okay to convert to async. I think this is only in cli.js, not CLIEngine, right? If so, I don't think that's part of the public API.

[kaicataldo]

I think we should try for synchronous first, but if that's not possible, I think it would be okay to convert to async. I think this is only in cli.js, not CLIEngine, right? If so, I don't think that's part of the public API.

Agreed!

[kaicataldo]

@jamesgeorge007 The current test is failing because cli.execute() is synchronous and doesn't return a promise, so there's nothing to await. As a result, it's not waiting for envinfo.run() to resolve before making its assertions. Do @platinumazure and my comments above make sense? Happy to clarify, if not!

[kaicataldo]

@jamesgeorge007 It's difficult for us to know what was going on without seeing the actual code.

Given the fact that it does so much and that it only has an async API, I'm wondering if using envinfo is overkill for this. Since the information we're after is accessible shelling out with child_process.execSync() and using the built in os module, thoughts on writing our own utility to get this information?

I also think we should really nail down exactly what information we're showing here for the ease of the PR author. We originally discussed including the information required for a bug report, which would include:

• ESLint Version:
• Node Version:
• npm Version:

Additionally, we could potentially add both a check for local and global installations and report them both:

• Local ESLint Version:
• Global ESLint Version:
• Node Version:
• npm Version:

Would it make sense to just start there? We can always add more information (the operating system, etc.) at a later date if we feel like it would be useful.

[aladdin-add]

some of the local-installed packages were used by eslint internally. e.g.

 * eslint-release: ^1.2.0 => 1.2.0 
 * eslint-scope: ^5.0.0 => 5.0.0 
 * eslint-utils: ^1.3.1 => 1.4.0 
 * eslint-visitor-keys: ^1.0.0 => 1.0.0 

is it possible to only show eslint-config-* and eslint-plugin-*?

[jamesgeorge007]

Updated info:-

Environment Information:-

  System:
    OS: Linux 4.18 Ubuntu 18.10 (Cosmic Cuttlefish)
    CPU: (2) x64 Intel(R) Pentium(R) CPU G2020 @ 2.90GHz
  Binaries:
    Node: 12.8.0 - /usr/local/bin/node
    Yarn: 1.13.0 - /usr/local/bin/yarn
    npm: 6.10.3 - /usr/local/bin/npm
  npmPackages:
    eslint-config-eslint: file:packages/eslint-config-eslint => 5.0.1 
    eslint-plugin-eslint-plugin: ^2.0.1 => 2.1.0 
    eslint-plugin-internal-rules: file:tools/internal-rules => 0.0.0 
    eslint-plugin-node: ^9.0.0 => 9.1.0 
  npmGlobalPackages:
    eslint: 6.1.0

[aladdin-add]

thanks for the update, seems local-installed eslint is missing? 😄

[mysticatea]

Hi.

• From bin/eslint.js#L69, it expects cli.execute returns a integer. We need to update it if we will go with Promises.
• From bin/eslint.js#L41, we need to watch unhandledRejection event to address unexpected errors if we will go with Promises..

[platinumazure]

@jamesgeorge007 Can you tell us what part you're having trouble with and what you've tried?

[jamesgeorge007]

• From bin/eslint.js#L69, it expects cli.execute returns a integer. We need to update it if we will go with Promises.
• From bin/eslint.js#L41, we need to watch unhandledRejection event to address unexpected errors if we will go with Promises..

It just says ,we need to update it in case of the promisified approach. Also, address the unhandledRejection event. All these are generic suggestions.
@platinumazure can you give me a better picture on what needs to be done further (specifically) 🤔

[mysticatea]

See the links I mentioned.

• See bin/eslint.js#L69.
  As it is, assigning the return value of cli.execute to process.exitCode. But you made cli.execute returning a Promise. What does it happen if you assign a Promise to process.exitCode? Because you changed the return value of a function, so you should modify the call side of the function.
• See bin/eslint.js#L41.
  ESLint CLI has the uncaughtException handler to catch unexpected errors and make proper messages and exit code. But this handler cannot catch exceptions in async logic. Because you added async logic newly, so you should add the handler that catches exceptions in async logic.

[jamesgeorge007]

The respective test case seem to fail 🤔

eslint/tests/bin/eslint.js

Line 170 in bfdb0c9

it("has exit code 0 if a linting warning is reported", () => assertExitCode(runESLint(["bin/eslint.js", "--env", "es6", "--no-eslintrc", "--rule", "semi: [1, never]"]), 0));

@platinumazure @mysticatea thoughts 🤔

[platinumazure]

Before we go too far down this path, I want to ask again: Did we identify a compelling reason for writing async code here, as well as using the envinfo dependency, rather than just rolling our own code (and hopefully being able to do so synchronously)?

I'm a little worried about the scope of this change and how difficult testing might become here, and I really think we should see if we can avoid the extra complexity here if at all possible.

[jamesgeorge007]

@platinumazure It's just that we wanna execute platform specific shell commands to fetch those information and why reinventing the wheel when everything is already cooked up 🤔
Still I'm of this thought 😕
Can you give me a better picture regarding how you're planning to implement an in-house solution for this one ❓ (implementation part)

[kaicataldo]

I’m a bit concerned that the team’s concerns aren’t being heard here. There are multiple comments above expressing concern about making the CLI code asynchronous and it seems to have been mostly ignored.

@platinumazure It's just that we wanna execute platform specific shell commands to fetch those information and why reinventing the wheel when everything is already cooked up 🤔
Still I'm of this thought 😕
Can you give me a better picture regarding how you're planning to implement an in-house solution for this one ❓ (implementation part)

From above:

It looks like in general the team is in agreement (@mysticatea do you mind clarifying your stance?) that we would like to take the approach of trying to do this synchronously, since conditionally returning a Promise leads to hard to debug code (as I think has been demonstrated by the challenges presented in this PR) and changing the CLI logic to be asynchronous is a task that we should evaluate separately (so we do our due diligence and make sure there aren't other unintended consequences of doing so).

Please let us know if you’d like us to clarify anything.

As far as the implementation goes, this would have to be explored by whoever wants to implement this, but please see this comment that outlines my take on how this might be implemented.

[jamesgeorge007]

@kaicataldo @platinumazure b6fb32 manages to take away the all confusion and hops over to the intended synchronous implementation.

With that we've
eslint -i

 Environment Information:-

 node: v12.8.0

 npm: 6.10.3

 eslint: v6.1.0

[jamesgeorge007]

The above mentioned information is as per the current ISSUE_TEMPLATE. Fetching local package information may require couple of workarounds 🤔

[kaicataldo]

I quickly spiked a synchronous alternative to the asynchronous version found in this PR to better illustrate what I had in mind my comments. I know there's been a lot of back and forth on here, and I'd love to see if we can come to a consensus and land this feature!

You can find the code here: kaicataldo@ef0fe91

This would print out the following log:

Environment Info:

Node version: 12.8.0
npm version: 6.10.3
Local ESLint version: 6.1.0 (Currently used)
Global ESLint version: 5.16.0

One key difference (and one I'd like to advocate for regardless of the implementation), is that when the --info flag is used, ESLint will not run the linting process. I think this makes more sense for the problem we've set out to solve here, which is to give users a quick and easy way to determine and share what versions of packages and binaries they have in their environment. By isolating the output, users won't have to dig through all the output of a linting job.

Note that I've only run this in macOS and so can't guarantee it'll work across other platforms in its current iteration.

[jamesgeorge007]

@kaicataldo LGTM 👍
I wonder why did you prefer to use cross-spawn while it could be done with the native child_process 🤔 (b6fb32)

[jamesgeorge007]

Also, how're we gonna proceed with this just to ensure my work here so far is not in vain 😅

[kaicataldo]

@jamesgeorge007 I used cross-spawn because it's what we use in lib/init/npm-utils.js. It looks like it handles some Windows-specific behavior (though I'm not super familiar with the specifics!).

As far as how we want to proceed - we should all decide! My intention was not to take over the issue. I just wanted to better illustrate what I had in mind, and you're welcome to continue working on this PR if you would like. That being said, I'm happy to clean up my branch, add tests, and make it a real PR, if that's deemed the appropriate path forward.

Regardless, I don't think your work has been in vain - you proposed this idea and made a PR that started a lot of discussion, and that's really valuable. 👍

[ilyavolodin]

@jamesgeorge007 We always appreciate contributions from the community, and prefer them to contributions from core team members. So if you are willing to incorporate @kaicataldo ideas into your PR, we would gladly merge that in.

[jamesgeorge007]

closing in favor of #12270