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

Move typescript to peer dependencies #350

Merged
merged 2 commits into from
Jan 29, 2023
Merged

Conversation

kamiazya
Copy link
Collaborator

@kamiazya kamiazya commented Jan 28, 2023

What was the problem

Currently, the typescript package is used to load tsconfig and is not used by users who only use JavaScript.

The implementation is such that require('typescript') is actually done in the function, so it does not affect all users.

However, including it in the dependencies may restrict the version of TypeScript that can be used, and a warning may be displayed if a user specifies a different version when installing madge.

Solution

Move the typescript dependency to the "peerDependencies" field and make the version specification a wildcard.

Also add the "peerDependenciesMeta" field and make typescript as optional.

This is to avoid conflicts with the user's specification, and also because of the version specification in the directive detective-typescript, we expect to use the typescript version specified by the detective-typescript package, or the typescript version specified by the user, to be used when resolving the version. or the user-specified typescript version is expected to be used.

"peerDependencies" that specify only typescript: * can be ignored during development, so we specify typescript@^4.5.5, which is the same as the [email protected] package.

@kamiazya kamiazya requested a review from pahen January 28, 2023 16:10
@kamiazya kamiazya marked this pull request as draft January 29, 2023 02:34
@kamiazya kamiazya removed the request for review from pahen January 29, 2023 02:35
@kamiazya
Copy link
Collaborator Author

Since it might be better to use peerDependencies and peerDependenciesMeta, we have returned PR to Draft for a while longer to consider how to handle this.

https://classic.yarnpkg.com/en/docs/package-json/#toc-peerdependencies

@PabloLION
Copy link
Collaborator

From what I read in NPM Docs, I'd say an optional peer dependency suits better here.

@kamiazya kamiazya changed the title Move typescript to optional dependencies Move typescript to peer dependencies Jan 29, 2023
@kamiazya kamiazya self-assigned this Jan 29, 2023
@kamiazya kamiazya requested a review from pahen January 29, 2023 07:13
@kamiazya kamiazya marked this pull request as ready for review January 29, 2023 07:13
@kamiazya
Copy link
Collaborator Author

The revised policy has been changed, as it seems more appropriate to specify in peerDepenedencies.

Copy link
Collaborator

@PabloLION PabloLION left a comment

Choose a reason for hiding this comment

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

LGTM

@PabloLION PabloLION merged commit 50bc474 into master Jan 29, 2023
@kamiazya kamiazya deleted the move-typescript-to-optional branch January 30, 2023 08:19
@kamiazya
Copy link
Collaborator Author

Previous studies: #275

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