-
Notifications
You must be signed in to change notification settings - Fork 0
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
#22 tslint eslint migration #36
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # .github/workflows/main.yml # libs/aws/.lintstagedrc.yml # libs/aws/package.json # libs/components/.lintstagedrc.yml # libs/components/package.json # libs/core/.lintstagedrc.yml # libs/core/package.json # package-lock.json # package.json
# Conflicts: # .github/workflows/main.yml # libs/aws/.lintstagedrc.yml # libs/aws/package.json # libs/components/.lintstagedrc.yml # libs/components/package.json # libs/core/.lintstagedrc.yml # libs/core/package.json # package-lock.json # package.json
…#22-tslint-eslint-migration # Conflicts: # .github/workflows/main.yml # libs/aws/.lintstagedrc.yml # libs/aws/package.json # libs/components/.lintstagedrc.yml # libs/components/package.json # libs/core/.lintstagedrc.yml # libs/core/package.json # package-lock.json # package.json
- @shiftcode/[email protected] - @shiftcode/[email protected] - @shiftcode/[email protected]
…#22-tslint-eslint-migration
- @shiftcode/[email protected] - @shiftcode/[email protected] - @shiftcode/[email protected]
…#22-tslint-eslint-migration
- @shiftcode/[email protected] - @shiftcode/[email protected] - @shiftcode/[email protected]
…#22-tslint-eslint-migration # Conflicts: # libs/components/package.json
- @shiftcode/[email protected] - @shiftcode/[email protected] - @shiftcode/[email protected]
- @shiftcode/[email protected] - @shiftcode/[email protected] - @shiftcode/[email protected]
- @shiftcode/[email protected] - @shiftcode/[email protected] - @shiftcode/[email protected]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All in all looks good but there are some minor changes to be implemented. Also be sure to 100% understand your package.json files and the dependency and resolutions
libs/components/src/lib/click-outside/click-outside.directive.ts
Outdated
Show resolved
Hide resolved
- @shiftcode/[email protected] - @shiftcode/[email protected] - @shiftcode/[email protected]
…#22-tslint-eslint-migration
- @shiftcode/[email protected] - @shiftcode/[email protected] - @shiftcode/[email protected]
- @shiftcode/[email protected] - @shiftcode/[email protected] - @shiftcode/[email protected]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sure to use @ts-expect-error
instead of @ts-ignore
where we used it before.
@@ -1,4 +1,3 @@ | |||
// tslint:disable:no-host-metadata-property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is eslint
not complaining about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelwittwer
Since we allowed that in our ng-config
https://github.com/shiftcode/sc-commons-public/blob/main/packages/eslint-config-recommended/src/ng-config/index.ts
@@ -17,7 +7,6 @@ import { Subscription } from 'rxjs' | |||
*/ | |||
@Directive({ selector: '[scClickOutside]', standalone: true }) | |||
export class ClickOutsideDirective implements OnDestroy, OnChanges { | |||
// tslint:disable-next-line:no-input-rename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is eslint
not complaining about that, compared to tslint
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelwittwer
Since we don't change the name, that existing input name if I change it to something else we get an error
libs/components/src/lib/tooltip/flexible-connected-position-strategy-2.ts
Outdated
Show resolved
Hide resolved
libs/components/src/lib/tooltip/flexible-connected-position-strategy-2.ts
Outdated
Show resolved
Hide resolved
@@ -1,5 +1,3 @@ | |||
// tslint:disable:use-host-property-decorator | |||
// tslint:disable:no-host-metadata-property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eslint
should also complain or is this allowed in our default config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelwittwer
Yes we allowed that
'@angular-eslint/no-host-metadata-property': 'off',
'@angular-eslint/no-host-metadata-property': 'off',
@@ -110,7 +114,7 @@ export class JwtHelper { | |||
return true | |||
} else { | |||
const date = this.getTokenExpirationDate(token) | |||
|
|||
// eslint-disable-next-line eqeqeq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use ===
since this.getTokenExpirationDate
will return null
or a Date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelwittwer isn't it better to use !date
instead?
a5091f8
to
1216b96
Compare
- @shiftcode/[email protected] - @shiftcode/[email protected] - @shiftcode/[email protected]
Tested on the pulse, the build was without errors.
Kinda mess with commits, needs to be merged with Squash or I can combine a few commits.
By the way, chore commit naming isn't allowed here