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

Document rejection of PackageJsonExtras and add tip for extending PackageJson #372

Merged
merged 3 commits into from
Mar 16, 2022

Conversation

pting-me
Copy link
Contributor

@pting-me pting-me commented Mar 7, 2022

Fixes #371

Initial PR to enhance the PackageJson type. Added a new type as proposed, named PackageJsonExtras.

Copy link
Collaborator

@voxpelli voxpelli left a comment

Choose a reason for hiding this comment

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

I'm skeptical in general.

Instead of introducing anything like this, you can simply in your own project do:

type MyPackageJson = PackageJson & { eslintConfig: import('eslint').Linter.Config };

Not sure adding it in here makes for any simpler or better implementation.

package.json Outdated
@@ -33,6 +33,7 @@
],
"devDependencies": {
"@sindresorhus/tsconfig": "~0.7.0",
"@types/eslint": "^8.4.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

For this to work, @types/eslint would have to be a dependency rather than a dev dependency.

Though I don't like the idea of this module starting to rely on lots of other type modules. I think it should be self-contained.

We don't know if the version of eslint used in the project is 8.x, 7.x or something else. We don't even know if they use eslint at all.

/**
ESLint configuration options.
*/
eslintConfig?: Linter.Config;
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a hack where we don't have to include the ESLint module in our dependencies:

Suggested change
eslintConfig?: Linter.Config;
// @ts-ignore
eslintConfig?: import('eslint').Linter.Config;

Then it will use import('eslint').Linter.Config if it correctly finds it, else it will just fallback to any.

I used that pattern a lot through ts-ignore-import and its the one pattern I think we maybe could use to still include this here. And if we do, then we can include it in PackageJson straight away rather than in a PackageJsonExtras. Thoughts @sindresorhus?

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this is a good pattern. Falling back to any is never something I would want. It could accidentally allow specifying anything there without a warning. Maybe if it fell back to never.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a really interesting pattern. I couldn't figure out how to override the any type with never (or anything other than any for that matter).

Side note, apparently @types/eslint is included in @typescript-eslint so I had to manually delete the package to test it. But yeah, probably best to leave versioning to the consumer.

@sindresorhus
Copy link
Owner

I think this is a very good reason not to add this:

We don't know if the version of eslint used in the project is 8.x, 7.x or something else.

@sindresorhus
Copy link
Owner

I suggest we just document:

import type {PackageJson as BasePackageJson} from 'type-fest';
import type {Linter} from 'eslint';

type PackageJson = BasePackageJson & {eslintConfig: Linter.Config};

Then it's clear to user how to support other tools too.

@pting-me
Copy link
Contributor Author

@sindresorhus Where would that go? Under "Declined Types"?

@pting-me pting-me force-pushed the package-json-eslint-config branch from 557b79a to 8ddaac9 Compare March 12, 2022 15:42
@pting-me
Copy link
Contributor Author

Reverted my changes and added some documentation based on the discussion.

@pting-me pting-me changed the title Add PackageJsonExtras type which includes property of eslintConfig Update README with rejection of PackageJsonExtras, and add tip for extending PackageJson Mar 13, 2022
@sindresorhus sindresorhus changed the title Update README with rejection of PackageJsonExtras, and add tip for extending PackageJson Document rejection of PackageJsonExtras and add tip for extending PackageJson Mar 16, 2022
@sindresorhus sindresorhus merged commit 716b8b2 into sindresorhus:main Mar 16, 2022
@sindresorhus
Copy link
Owner

Thanks :)

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.

Add eslintConfig to PackageJson type
3 participants