Update typescript #109
Update typescript #109
Conversation
There was a problem hiding this comment.
Lots of great changes in this! However, I think we can remove more rule disablements. It should be very rare when we disable rules and there should be a reason provided when we do....unless we're planning to file more stories about removing rule disablements.
Not sure where things are with the lazy loader code. Maybe @amphro has a better sense of the status of that lib.
packages/kit/test/env.test.ts
Outdated
| @@ -9,6 +9,8 @@ import { expect } from 'chai'; | |||
| import { Env } from '../src/env'; | |||
| import { InvalidDefaultEnvValueError } from '../src/errors'; | |||
|
|
|||
| /* eslint-disable no-shadow */ | |||
There was a problem hiding this comment.
IMO we should change the code to not shadow.
packages/kit/src/duration.ts
Outdated
| @@ -280,6 +280,7 @@ export namespace Duration { | |||
| /** | |||
| * Units of duration. | |||
| */ | |||
| /* eslint-disable-next-line no-shadow */ | |||
There was a problem hiding this comment.
Can we just remove the shadowed vars?
packages/kit/src/creatable.ts
Outdated
| @@ -5,6 +5,9 @@ | |||
| * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause | |||
| */ | |||
|
|
|||
| /* eslint-disable @typescript-eslint/ban-types */ | |||
| /* eslint-disable no-shadow */ | |||
There was a problem hiding this comment.
What are the types we're using that it doesn't like? Can't we fix that instead? And refactor the shadowed vars?
packages/kit/src/nodash/external.ts
Outdated
| @@ -69,7 +72,8 @@ export function defaults(obj: unknown, ...otherArgs: unknown[]): unknown { | |||
| * @param predicate The function invoked per iteration. | |||
| */ | |||
| export function findKey<T>(obj: Nullable<T>, predicate?: ObjectIteratee<T>): Optional<string> { | |||
| return _.findKey(obj, predicate); | |||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | |||
There was a problem hiding this comment.
This doesn't seem necessary anymore since it's disabled for the entire file above. However, we should do what we can to avoid disabling typescript rules. If we really have to, then add a comment to describe why it's necessary.
| * Typescript recommends using Record<string, unknown> instead of the generic object type. See https://github.com/microsoft/TypeScript/issues/21732. | ||
| * However, this breaks current functionality so we aliased the object type and disabled eslint for now. | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/ban-types |
There was a problem hiding this comment.
Should we create a work item to make this change in a major version bump of the library?
| @@ -39,6 +39,7 @@ export function toAnyJson<T>(value: Nullable<T>, defaultValue: AnyJson): AnyJson | |||
| // underlying function | |||
| export function toAnyJson<T>(value: Nullable<T>, defaultValue?: AnyJson): Optional<AnyJson> { | |||
| try { | |||
| /* eslint-disable-next-line @typescript-eslint/no-unsafe-return */ | |||
There was a problem hiding this comment.
Can't we just coerce the type returned by JSON.parse() to be AnyJson rather than disable the rule?
mdonnalley
force-pushed
the
mdonnalley/update-ts
branch
from
ea4a484 to
a5f0810
Compare
mdonnalley
force-pushed
the
mdonnalley/update-ts
branch
3 times, most recently
from
071900b to
11ca471
Compare
mdonnalley
requested review from
WillieRuemmele,
mshanemc,
peternhale and
rorolopetegui
| "@typescript-eslint/eslint-plugin": "^2.30.0", | ||
| "@typescript-eslint/parser": "^2.30.0", | ||
| "@typescript-eslint/eslint-plugin": "^4.2.0", | ||
| "@typescript-eslint/parser": "^4.2.0", | ||
| "eslint": "^6.8.0", |
There was a problem hiding this comment.
Geesh, I wonder if we also need a similar story around keeping eslint up-to-date... it is never ending.
mdonnalley
force-pushed
the
mdonnalley/update-ts
branch
from
11ca471 to
ea52ae2
Compare
mdonnalley
force-pushed
the
mdonnalley/update-ts
branch
from
ea52ae2 to
ec3b8cd
Compare
|
I ran and tested this locally, as well as linked it to several projects. The new versions of kit and ts-types works great. The new dev scripts cause a lot of compilation problems in new repos but that is expected with a major bump of typescript, but that is only a dev dep. |
Updates
4.1.3^4.2.0^4.2.0@W-8667867@