aurelia / tools-ts

The shared set of tools that are used for TypeScript library builds.
MIT License
4 stars 1 forks source link

chore(tsconfig): add tsconfigs #2

Open StrahilKazlachev opened 6 years ago

StrahilKazlachev commented 6 years ago

This is based on the configs of aurelia-validation, aurelia-dialog, aurelia-ux.

  1. I skipped noUnusedLocals, noUnusedParameters - I would leave this to the linter
  2. We were already using the majority of the strict settings so I just added strict: true. We should be cautious with this though since new flags can be added with new TS versions.
  3. The only problem that popped up when I applied it to aurelia-dialog was related to the noImplicitThis setting - in a PropertyDescriptor get/set needed explicit this typing.
  4. Every inheriting project will have to add its exclude, include, ... - the patterns are resolved relative to the file they are declared. Did add a reference excludes - if the hackish comment in the .jsons is too much or will result in some problems I'll remove it.
jods4 commented 6 years ago
  1. noUnusedLocals and noUnusedParameters

Why not leave them on if this is what we want? Typescript gives immediate feedback in the IDE, the sooner we see our mistakes the better.

I usually keep noUnusedLocals but not noUnusedParameters because there are cases where you legitimately want a function that has unused parameters (inheritance, interfaces, etc.)

That said, maybe I'll change my mind because TS 2.6 has introduced a way to suppress errors with // @ts-ignore, which makes noUnusedParameters more usable -> document why a parameter is there if you don't use it.

  1. We should be cautious with this though since new flags can be added with new TS versions.

If there's a new version introduces a flag you don't like in strict (2.6 introduced strictFunctionTypes), we can always turn it off like so: strict: true, strictFunctionTypes: false. Setting an option to false takes precedence over strict: true.

  1. noImplicitThis

Was this a real problem or you just needed to add a type annotation for this? This option has caught many problems in code at work, caused by the highly dynamic nature of this in JS. Static validation is good...

  1. exclude and include

If you put your tsconfig at the root of your src then TS defaults to all .ts files inside it and its subfolder. That's how I usually does it and it shouldn't require exclude nor include usage.

  1. Can we add safety option noFallthroughCasesInSwitch?

  2. @autoinject needs emitDecoratorMetadata but it generates lots of unneeded metadata (everywhere any decorator is used). I think we can agree not to use any of those in the core Aurelia libs?

  3. stripInternal is nice for a lib like aurelia, it enables us to have "internal" apis. By using comment /** @internal */ a public API (for internal usage) won't be documented in the generated .d.ts.

  4. This one will be controversial but anyway: I usually have suppressImplicitAnyIndexErrors: true. This means access like x["blah"] will not create an error if there's no blah defined and will be typed as any. This is a very common pattern in bag/dictionary/option objects and I like having that escape hatch without explicit casting to any. If you want statically typesafe code, it feels natural to use x.blah.

StrahilKazlachev commented 6 years ago

@jods4

I'll await for some more input before making changes.

jods4 commented 6 years ago

stripInternal -> why not in the base one? After all if you don't use /** @internal */ it has no effect. Do you think there is a chance someone uses the comment when it shouldn't if we turn the TS option on? It can always get turned off at the project level.

jdanyow commented 6 years ago

I vote for strict:true. We can address issues that crop up as a result of new checks when we update the package’s typescript dependency.

StrahilKazlachev commented 6 years ago

@jods4 my intention was to have the compiler options somewhat semantically separated - you are doing dist build, use/extend tsconfig.build.json. And options like stripInternal, declaration, removeComments(not using because of the docs) for me are meaningful only for the dist/api doc build. We could also have only 1 tsconfig.json, if that's more logical/practical.

jods4 commented 6 years ago

@StrahilKazlachev OK that was unclear to me. Sure, if we have a base tsconfig for build and one for docs, we can put stripInternal in the docs one.

StrahilKazlachev commented 6 years ago

@jods4

jods4 commented 6 years ago

At aurelia-pal I've added tslint-language-service as a compiler plugin. This enables live feedback in IDE when your code doesn't properly lint: image

And code fixes! image

EisenbergEffect commented 6 years ago

How's this looking?