Skip to content
This repository has been archived by the owner on Mar 8, 2020. It is now read-only.

Sanitise dependencies #3798

Merged
merged 5 commits into from
Apr 27, 2018

Conversation

mbwhite
Copy link
Contributor

@mbwhite mbwhite commented Apr 10, 2018

Introduce peerDependencies for a number of core modules to reduce the size of them.

part resolves #3795

@mbwhite mbwhite requested a review from sstone1 April 10, 2018 15:29
"dependencies": {
"composer-common": "0.19.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is not in devDependencies like the other ones?

"dependencies": {
"composer-common": "0.19.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is not in devDependencies like the other ones?

"dependencies": {
"composer-common": "0.19.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is not in devDependencies like the other ones?

Copy link
Contributor

@sstone1 sstone1 left a comment

Choose a reason for hiding this comment

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

@mbwhite could you look at the comments relating to a few packages missing composer-common from devDependencies? I guess I don't see why it's required for some packages but not others - might be a good reason for it though!

@mbwhite
Copy link
Contributor Author

mbwhite commented Apr 18, 2018

@sstone1
good question; devDepenendencies need to have composer-common when whatever function the tests invoke, actually need composer-common. In the cases you've mentioned - these tests are not invoking composer-common code so don't need the devDedependency.

sstone1
sstone1 previously approved these changes Apr 18, 2018
Copy link
Contributor

@sstone1 sstone1 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@sstone1
Copy link
Contributor

sstone1 commented Apr 20, 2018

This doesn't build @mbwhite!

@sstone1 sstone1 dismissed their stale review April 20, 2018 13:38

Does not build!

@mbwhite
Copy link
Contributor Author

mbwhite commented Apr 20, 2018

@sstone1 not yet no...

@mbwhite mbwhite force-pushed the sanitise-dependencies branch 2 times, most recently from 103f89d to 06aef7d Compare April 27, 2018 09:13
Signed-off-by: Matthew B White <whitemat@uk.ibm.com>
Signed-off-by: Matthew B White <whitemat@uk.ibm.com>
Signed-off-by: Matthew B White <whitemat@uk.ibm.com>
Signed-off-by: Matthew B White <whitemat@uk.ibm.com>
Signed-off-by: Matthew B White <whitemat@uk.ibm.com>
@mbwhite
Copy link
Contributor Author

mbwhite commented Apr 27, 2018

Build works on my own travis org... https://travis-ci.org/mbwhite/composer/builds/371939672

@jt-nti jt-nti self-assigned this Apr 27, 2018
Copy link
Contributor

@jt-nti jt-nti left a comment

Choose a reason for hiding this comment

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

Peer dependencies sound awesome

@mbwhite mbwhite merged commit 02ea0e0 into hyperledger-archives:master Apr 27, 2018
@mbwhite mbwhite deleted the sanitise-dependencies branch April 27, 2018 11:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants