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

Eliminate @storis dependencies internalize configs #644

Merged
merged 21 commits into from
Apr 17, 2024

Conversation

philfuster
Copy link
Contributor

@philfuster philfuster commented Apr 16, 2024

This PR removes the two storis dependencies @storis/eslint-config and @storis/tsconfig. Those repositories are no longer being maintained, so their configs will be internalized.

This PR also updates the eslint-config-prettier plugin.

There was an attempt made to upgrade eslint to v9 as well, but there are plugins that are yet to support v9 of eslint. They have been documented in a tracker issue - #646

@philfuster philfuster changed the base branch from main to upgrade-dependencies April 17, 2024 14:00
@philfuster philfuster marked this pull request as ready for review April 17, 2024 15:09
Copy link
Member

@shawnmcknight shawnmcknight left a comment

Choose a reason for hiding this comment

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

Overall looks good. The biggest issues are some things that are not relevant to this repo that leaked into the configs.

.eslintrc.js Outdated
},
// allow trailing ASC and DESC on enumerations
{
selector: 'enumMember',
Copy link
Member

Choose a reason for hiding this comment

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

This rule is likely not relevant for this repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

.eslintrc.js Outdated
format: null,
},
// GraphQL variables PascalCase
{
Copy link
Member

Choose a reason for hiding this comment

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

This rule is likely not relevant for this repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

.eslintrc.js Outdated
},

{
files: ['**/*.types.ts', '**/types/*.ts', '**/*.schema.ts', '**/instances/**'],
Copy link
Member

Choose a reason for hiding this comment

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

I think the **/types/*.ts file override is relevant for this repo, but I don't believe any of the others are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

extends: ['plugin:@typescript-eslint/recommended-requiring-type-checking'],
rules: {
// ban ts-comment except with description
'@typescript-eslint/ban-ts-comment': [
Copy link
Member

Choose a reason for hiding this comment

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

I think we should make this:

				// ban ts-comment except with description
				'@typescript-eslint/ban-ts-comment': [
					'error',
					{
						'ts-expect-error': { descriptionFormat: '^: ' },
						'ts-ignore': true,
						'ts-nocheck': true,
						'ts-check': false,
					},
				],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Looking at all these rules has been great. Thank you.

.eslintrc.js Outdated
},

{
files: ['**/index.[jt]s?(x)', '**/*.gql.[jt]s', '**/constants.ts'],
Copy link
Member

Choose a reason for hiding this comment

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

The gql suffix is definitely not needed as an override. I don't think the constants one is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed gql and constants file matches.

tsconfig.json Outdated
},
"include": [
"src/**/*", // source folder
"scripts/**/*", // scripts folder
"test/**/*", // test helpers
"./*.js", // root javascript files
"./.*.js" // root javascript config files
]
],
"ts-node": {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't relevant to this repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. removed ts-node config

@philfuster philfuster changed the base branch from upgrade-dependencies to main April 17, 2024 19:43
Copy link
Member

@shawnmcknight shawnmcknight left a comment

Choose a reason for hiding this comment

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

LGTM!

@shawnmcknight shawnmcknight merged commit 6a93812 into main Apr 17, 2024
4 checks passed
@shawnmcknight shawnmcknight deleted the eliminate-@storis-dependencies-internalize-configs branch April 17, 2024 21:09
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.

2 participants