Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

[#371] relax tsutils peer dependency #371

Merged
merged 1 commit into from
May 11, 2017
Merged

Conversation

sapegin
Copy link
Contributor

@sapegin sapegin commented May 11, 2017

To prevent warning when using newer tsutils version:

warning "[email protected]" has incorrect peer dependency "[email protected]".

@msftclas
Copy link

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@HamletDRC HamletDRC changed the title Relax tsutils peer dependency [#371] relax tsutils peer dependency May 11, 2017
@HamletDRC HamletDRC merged commit 648f1fb into microsoft:master May 11, 2017
@Delagen
Copy link

Delagen commented Jun 7, 2017

@msftclas I still think that this must be full dependency.
NPM 2 or NPM from 3 to 5 with legacy-bundling will be broken if tsutils missing in package.json of main project (only tslint and tslint-microsoft-contrib)

@HamletDRC
Copy link
Member

@Delagen I don't think I should make it a full dependency. tslint-microsoft-contrib is just a folder that tslint calls into. It does not have any actual dependencies, it only inherits dependencies from tslint. Even if I added a full dependency, I don't think it would be used. tslint would load itself and the tsutils modules, and then load one of our rule files from disk.

What I can do is delete the peer dependency entirely. This is a transitive dependency satisfied by tslint, and we need to just rely on whatever tslint gives us.

@sapegin if I just delete the tslint peer dependency then will this still work for you?

@sapegin
Copy link
Contributor Author

sapegin commented Jun 19, 2017

I actually agree with @Delagen — for me it’s like Lodash or any other utility library, I’d put it into dependencies myself, just use ^ to use the same version that ESLint may use.

@sapegin sapegin deleted the tsutils branch June 19, 2017 13:51
@HamletDRC
Copy link
Member

@sapegin @Delagen
I had a misunderstanding of what tsutils is.

I think it should be a full dependency.

Can you point your project at our #releases branch and see if this fixes the issue for you?

@HamletDRC
Copy link
Member

@sapegin does thumbs up mean you tested it? Nothing was ever failing for me, so all my tests pass regardless.

@sapegin
Copy link
Contributor Author

sapegin commented Jun 20, 2017

@HamletDRC Works fine for me!

@westy92
Copy link

westy92 commented Jun 22, 2017

It looks like this was reverted when switching from a peerDependency to a dependency.
d33dbf6
Does anybody know why?

@HamletDRC
Copy link
Member

@westy92 As per the discussion above, we are looking to move this from a peerDependency to a dependency.

Is there a reason why it needs to be a peerDependency and a dependency?

I assumed the commit you reference is the correct way to do it.

cc @sapegin @Delagen

@westy92
Copy link

westy92 commented Jun 23, 2017

It should be a dependency and not a peerDependency.
This PR changed it from a strict version to >=1.6.0, but when it was moved from a peerDependency to a dependency, the >= was changed to ^. I believe it should be fixed to again use >= to re-fix what this PR accomplished, "prevent warning when using newer tsutils version."

@sapegin
Copy link
Contributor Author

sapegin commented Jun 23, 2017

It will warn only when using a new major version.

@westy92
Copy link

westy92 commented Jun 23, 2017

I understand, it just seems like a step backwards to go from >=1.6.0 (no warning) to ^1.4.0 (warning).

@sapegin
Copy link
Contributor Author

sapegin commented Jun 23, 2017

Actually having >= is not that safe and useful — it may just break without an npm warning if a new major release isn’t backwards compatible ;-)

@sapegin
Copy link
Contributor Author

sapegin commented Jun 23, 2017

And according to their 2.0.0 change log that is very likely. So I think we should use ^2.0.0 here.

@HamletDRC HamletDRC added this to the 5.0.1 milestone Jun 27, 2017
@HamletDRC
Copy link
Member

Triggering a release now... expect something in npm by end of day.

@reduckted
Copy link
Contributor

So I think we should use ^2.0.0 here.

I agree. Is there any reason why it's still using 1.6.0?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants