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

Explicit peer dependencies #108

Merged
merged 2 commits into from
Jul 18, 2017
Merged

Explicit peer dependencies #108

merged 2 commits into from
Jul 18, 2017

Conversation

adidahiya
Copy link
Contributor

@adidahiya adidahiya commented Jul 18, 2017

Fixes #106

@adidahiya adidahiya merged commit 35f49a5 into master Jul 18, 2017
@adidahiya adidahiya deleted the ad/peer-deps branch July 18, 2017 03:28
@timocov
Copy link

timocov commented Jul 18, 2017

@adidahiya why tsutils is in peerDependencies instead of be in dependencies?

@timocov
Copy link

timocov commented Jul 18, 2017

I guess it is similar to microsoft/tslint-microsoft-contrib#371

@adidahiya
Copy link
Contributor Author

Meh, this is kind of the reason I never listed explicit peer dependencies in the first place (it gets tedious to maintain and doesn't provide many benefits). The whole idea here is to prevent multiple versions of tsc from being used in TSLint and its rule implementations at runtime. If I make tsutils a regular dependency then I believe there's a possibility that NPM could install a nested typescript module inside tsutils/node_modules and now we have two versions in use at runtime (despite the fact that typescript itself is a peer dependency of tslint-react) -- I'll need to double-check this.

@timocov
Copy link

timocov commented Jul 19, 2017

I agree, it is good reason.

If I make tsutils a regular dependency then I believe there's a possibility that NPM could install a nested typescript module inside tsutils/node_modules

But typescript is a peer dependency of tsutils (https://github.com/ajafff/tsutils/blob/master/package.json#L43) and I guess that if you use a library with specific API for specific range of versions you should make it as dependency.

@timocov
Copy link

timocov commented Jul 19, 2017

And now with [email protected] we have an error about not-installed tsutils for tslint-react (yeah, it is bug of npm but I guess package.json of tslint-react should has tsulits as deps)

@adidahiya
Copy link
Contributor Author

@ajafff do you have any opinions here?

@ajafff
Copy link

ajafff commented Jul 19, 2017

As @timocov said, tsutils has typescript in peerDependencies. Hence there will only be one version of typescript installed.

The only reason to put tsutils in peerDependencies is to avoid different (major) versions of it in tslint and rules packages. But I don't think that's necessary.

@adidahiya
Copy link
Contributor Author

adidahiya commented Jul 19, 2017

@ajafff given that logic, it should probably be safe to make tslint also a regular dependency here then? Since it also declares typescript as a peer.

@timocov
Copy link

timocov commented Jul 20, 2017

I think that tslint should be peer dependency because tslint-react is plugin for tslint. Yes, tslint-react depends on tslint but in development only and just for typings. (https://nodejs.org/en/blog/npm/peer-dependencies/)

@ajafff
Copy link

ajafff commented Jul 27, 2017

I agree with @timocov. A plugin should not bring it's own version of the library it plugs into.

Take vrsource-tslint-rules for example: It has a regular dependency on tslint@^4.1.1. You can install this package even if you have tslint@^5.0.0 installed. You won't notice any incompatibility until you experience a runtime crash.

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.

3 participants