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

#22 tslint eslint migration #36

Open
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

vitaliibendyk
Copy link
Collaborator

@vitaliibendyk vitaliibendyk commented Aug 13, 2024

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

@vitaliibendyk vitaliibendyk linked an issue Aug 13, 2024 that may be closed by this pull request
vitaliibendyk and others added 8 commits August 13, 2024 22:18
# 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
@vitaliibendyk vitaliibendyk removed the request for review from michaelwittwer August 26, 2024 10:04
@vitaliibendyk vitaliibendyk marked this pull request as ready for review August 26, 2024 10:57
@vitaliibendyk vitaliibendyk marked this pull request as draft August 26, 2024 11:06
@vitaliibendyk vitaliibendyk marked this pull request as ready for review August 27, 2024 10:18
Copy link
Member

@arisjavet arisjavet left a 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/.lintstagedrc.yml Outdated Show resolved Hide resolved
libs/components/.eslintrc.cjs Show resolved Hide resolved
libs/components/src/lib/rx/rx-if.directive.ts Outdated Show resolved Hide resolved
libs/core/ng-package.json Outdated Show resolved Hide resolved
libs/core/src/lib/static-utils/jwt-helper.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
scripts/ts/copy-version.ts Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
libs/components/.eslintrc.cjs Outdated Show resolved Hide resolved
libs/components/.lintstagedrc.yml Outdated Show resolved Hide resolved
libs/core/.eslintrc.cjs Outdated Show resolved Hide resolved
libs/core/src/lib/static-utils/jwt-helper.ts Outdated Show resolved Hide resolved
libs/components/src/lib/tooltip/tooltip.directive.ts Outdated Show resolved Hide resolved
Copy link
Member

@michaelwittwer michaelwittwer left a 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.

libs/aws/src/lib/cloudwatch-logger/is-error.function.ts Outdated Show resolved Hide resolved
@@ -1,4 +1,3 @@
// tslint:disable:no-host-metadata-property
Copy link
Member

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -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
Copy link
Member

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?

Copy link
Collaborator Author

@vitaliibendyk vitaliibendyk Dec 12, 2024

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
Screenshot 2024-12-12 at 15 28 23

@@ -1,5 +1,3 @@
// tslint:disable:use-host-property-decorator
// tslint:disable:no-host-metadata-property
Copy link
Member

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?

Copy link
Collaborator Author

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
Copy link
Member

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

Copy link
Collaborator Author

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?

libs/core/src/lib/ui-events/ui-event.service.ts Outdated Show resolved Hide resolved
scripts/ts/copy-version.ts Outdated Show resolved Hide resolved
lerna.json Outdated Show resolved Hide resolved
@vitaliibendyk vitaliibendyk force-pushed the #22-tslint-eslint-migration branch from a5091f8 to 1216b96 Compare December 12, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tslint -> eslint
4 participants