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

noUnusedParameters always enabled by default when using typescript from node_modules #1433

Closed
Neonit opened this issue Jun 18, 2018 · 6 comments

Comments

@Neonit
Copy link

Neonit commented Jun 18, 2018

Problem

If Typescript is installed locally in node_modules, atom-typescript uses it instead of the version shipped with the package. This causes a behavior as if noUnusedParameters was enabled. Setting it explicitly to false didn't solve the problem for me.

Reproduction

I was using atom-typescript 12.5.3 when reproducing this like so:

  1. In an empty folder, open Atom and create a file foo.ts:
function foo(x: string)
{}
  1. No errors or warnings should be reported.
  2. Now open a console in that folder and install Typescript locally: npm i typescript.
  3. Restart or reload (Ctrl + Shift + F5) Atom.
  4. Once the package has been initialized, the parameter x of foo should get a blue squiggly underline and the compiler should complain about an unused parameter.
  5. Running tsc foo.ts should not output any warnings or errors.
    (No tsconfig.json was used for reproduction)

Expectation

The expected behavior is that atom-typescript just plays by the rules set in the tsconfig.json, no matter whether it uses it's own or a locally installed version of Typescript. I haven't tested whether this affects any other settings.

@lierdakil
Copy link
Collaborator

This seems to be an issue with TypeScript 2.9, which apparently reports those diagnostics regardless of tsconfig settings.

I could not reproduce on TypeScript 2.8.

For additional context, these diagnostics are reported via suggestionDiag (which is a new feature in TypeScript v2.8)

If you could open an issue on Microsoft/TypeScript, that would be very helpful.

The best we could offer from our end is #1434, but it will take some time to polish

(we could of course ignore all suggestion-type diagnostics, but that would be throwing the baby out with the bathwater, as far as I'm concerned)

@Neonit
Copy link
Author

Neonit commented Jun 19, 2018

I would open an issue in the Typescript repo, but I lack the background and knowledge to fulfill all their requirements there. I just figured out suggestion diagnostics is a service that provides you in this case with the information that the parameters there are unused and shouldn't be?

They require a test with typescript@next in case the issue is already resolved in the newest version. I don't know how to make atom-typescript use typescript@next nor how to create a suitable minimal example without atom-typescript.

Preparing the issue as desired is out of my time frame, sorry. I'd be happy if someone with the necessary knowledge could help out here as this wouldn't take much of their time.

@lierdakil
Copy link
Collaborator

Hmm. Went into this rabbit hole, and traced this back to microsoft/TypeScript#22361 -- so apparently this is very deliberate.

@lierdakil
Copy link
Collaborator

Okay, so there is an "escape hatch" so to speak. I've added necessary checks to #1434. Will try to cook up a release this weak.

In the meantime, consider using TypeScript 2.8 if you can.

For the record, atom-typescript uses whatever TypeScript version you have installed into node_modules. If there isn't one, only then it will fallback to the bundled version, which happens to be TypeScript 2.8 at the moment.

@Neonit
Copy link
Author

Neonit commented Jun 19, 2018

Thanks for your engagement. Take your time, though. It's a bit annoying, but it doesn't hinder development.

@lierdakil
Copy link
Collaborator

v12.6.0 is now released 🎉 which includes the following setting:
image
(disabled by default for now, because I assume TypeScript team knows what they're doing, but it might be enabled by default later in a patch-release if more people complain)

Hopefully this is satisfactory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants