convert npm dependencies to peerDependencies by IgorMinar · Pull Request #5649 · angular/angular

IgorMinar · 2015-12-06T08:53:00Z

pkozlowski-opensource · 2015-12-06T09:44:06Z

package.json

Shouldn't we be more strict about rxjs? If I recall properly alpha.12 was kind of broken and @robwormald had to go back to .11. In any case given the amount of changes in both libraries we might be more strict here.

I'll do that in a separate commit within this PR. do we know what version of RXJS is the good one?

pkozlowski-opensource · 2015-12-06T09:46:54Z

Travis build is not a flake, I think that we read from package.json dependencies in some of our scripts

IgorMinar · 2015-12-06T17:34:31Z

yay for CI. I'll fix the breakage.

IgorMinar · 2015-12-06T18:12:20Z

@pkozlowski-opensource should be all fixed now. I also added es6-promise as a peer dependency (it comes from zone.js). waiting for CI now..

IgorMinar · 2015-12-06T18:21:57Z

oh.. I still need to lock rxjs.. one more commit coming.

IgorMinar · 2015-12-06T18:28:25Z

done

jeffbcross · 2015-12-06T19:21:28Z

package.json

Why do we need this now?

We always needed it. It's a transitive dep coming from zone
On Sun, Dec 6, 2015 at 11:21 AM Jeff Cross notifications@github.com wrote:

In package.json
#5649 (comment):

@@ -30,9 +30,11 @@
"postinstall": "node tools/analytics/build-analytics success install npm-install-net && node tools/analytics/build-analytics start install npm-postinstall && node tools/npm/copy-npm-shrinkwrap && node tools/chromedriverpatch.js && webdriver-manager update && bower install && gulp pubget.dart && tsd reinstall --overwrite --clean --config modules/angular2/tsd.json && tsd reinstall --overwrite --clean --config tools/tsd.json && tsd reinstall --overwrite --config modules/angular1_router/tsd.json && node tools/analytics/build-analytics success install npm-postinstall && node tools/analytics/build-analytics success install npm-install",
"test": "gulp test.all.js && gulp test.all.dart"
},

"dependencies": {

"peerDependencies": {

"es6-promise": "^3.0.2",

Why do we need this now?

—
Reply to this email directly or view it on GitHub
https://github.com/angular/angular/pull/5649/files#r46774522.

Isn't it part of zone.js build? Or are you saying that we need to have it for some other reason (typings)?

pkozlowski-opensource · 2015-12-07T10:04:43Z

@IgorMinar the more I look at this PR the more I think that we are focusing on the wrong package.json. What we want to do is to have peer dependencies in the package.json of a module that we publish to npm.

Here we still want things as regular (dependencies or devDependencies) deps since we need zone and company to run tests, prepare bundles etc.

What I'm saying is that we don't want to move zone & co to peereDependencies in this repo's package.json but do this only in package.json that we push to npm.

pkozlowski-opensource · 2015-12-07T10:24:14Z

package.json

Do we want to force es6-shim? What about people that do projects for modern browsers only? Or want to use a different shim?

jeffbcross · 2015-12-07T17:38:43Z

@pkozlowski-opensource: @IgorMinar said you are right (as always) and that he is going to update the PR to keep devDependencies in our own package, peerDependencies in published packages.

IgorMinar · 2015-12-07T17:48:54Z

@pkozlowski-opensource you are right. my bad. I'm fixing that now.

IgorMinar · 2015-12-07T17:49:08Z

ci still failing, looking into that too

IgorMinar · 2015-12-07T18:38:57Z

changes made. running CI. if green then I need to squash commits and we can land this

pkozlowski-opensource · 2015-12-07T18:43:42Z

@IgorMinar what about my comments regarding es6-promise and es6-shim?

I'm not sure why we need es6-promise (it is bundled with zone.js) and es6-shim is debatable IMO (what about people using recent browsers? Or ones using a different shim?)

jeffbcross · 2015-12-07T18:54:17Z

modules/angular2/src/http/package.json

Should have a dependency on rxjs

I guess rxjs is already declared as a peerDependency of angular2, so as long as this package depends on angular2 rxjs isn't necessary.

And http isn't even published as a standalone package yet.

Jeff, rxjs comes in via angular. But I'll make it an explicit peer dep.

Pawel, see my comment about optionalPeerDependencies in one of my commit
messages. For now apps that don't need shims will still need to declare
them in their package.json but not use them.
On Mon, Dec 7, 2015 at 11:36 AM Jeff Cross notifications@github.com wrote:

In modules/angular2/src/http/package.json
#5649 (comment):

"devDependencies": <%= JSON.stringify(packageJson.defaultDevDependencies) %>

"peerDependencies": {

"angular2": "<%= packageJson.version %>"

And http isn't even published as a standalone package yet.

—
Reply to this email directly or view it on GitHub
https://github.com/angular/angular/pull/5649/files#r46865671.

IgorMinar · 2015-12-08T00:59:01Z

CI is green! I'm squashing the commits and applying merge label

IgorMinar · 2015-12-08T14:47:56Z

everything is resolved and rebased. good to be merged now

jelbourn · 2015-12-08T17:43:02Z

@IgorMinar Travis failures still look legitimate to me.

IgorMinar · 2015-12-08T18:45:33Z

looking into it

To be later used as a peerDependency in the generated package.json It would be better to make this one an optionalPeerDependency but npm currently doesn't support making peerDependencies optional. See: npm/npm#3066

This is actually an inherited dependency that comes from zone.js. See related issue: angular/zone.js#212 It would be better to make this one an optionalPeerDependency but npm currently doesn't support making peerDependencies optional. See: npm/npm#3066

…age.json templates This is more correct and resolves the issue of having dupes within the same project. This change has no impact on our shrinkwrap since peerDeps and deps are merged into one within the shrinkwrap file. BREAKING CHANGE: rxjs, reflect-metadata, zone.js and es6-shims now must be specified as explicit dependencies of each angular app that uses npm for package management. To migrate, please add the following into the "dependencies" section of your package.json: ``` "dependencies": { ... "es6-promise": "^3.0.2", "es6-shim": "^0.33.3", "reflect-metadata": "0.1.2", "rxjs": "5.0.0-alpha.11", "zone.js": "0.5.8" ... } ``` Closes angular#5560

IgorMinar · 2015-12-08T18:48:11Z

sorry.. it was a bad squash. fixed now. rerunning CI

jelbourn · 2015-12-08T19:51:13Z

@mary-poppins isn't picking this one up, merging manually.

angular-automatic-lock-bot · 2019-09-07T16:21:50Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Dec 6, 2015

IgorMinar assigned jeffwhelpley and jeffbcross and unassigned jeffwhelpley Dec 6, 2015

IgorMinar added the action: review The PR is still awaiting reviews from at least one requested reviewer label Dec 6, 2015

IgorMinar added this to the beta.0 milestone Dec 6, 2015

pkozlowski-opensource reviewed Dec 6, 2015
View reviewed changes

IgorMinar force-pushed the peer-deps branch from 5a62c7d to f23731e Compare December 6, 2015 18:11

IgorMinar force-pushed the peer-deps branch from 1fc4370 to d6931fd Compare December 6, 2015 18:28

jeffbcross reviewed Dec 6, 2015
View reviewed changes

pkozlowski-opensource reviewed Dec 7, 2015
View reviewed changes

jeffbcross added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 7, 2015

jeffbcross assigned IgorMinar and unassigned jeffbcross Dec 7, 2015

jeffbcross mentioned this pull request Dec 7, 2015

docs(Observable): document how to compose operators #5642

Closed

jeffbcross reviewed Dec 7, 2015
View reviewed changes

jeffbcross added the pr_state: LGTM label Dec 7, 2015

IgorMinar force-pushed the peer-deps branch from 54dd087 to 24dddaf Compare December 8, 2015 14:38

IgorMinar added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Dec 8, 2015

IgorMinar force-pushed the peer-deps branch from 24dddaf to 5a851ba Compare December 8, 2015 14:47

jelbourn added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Dec 8, 2015

IgorMinar added 4 commits December 8, 2015 10:47

fix(npm): move es6-shim from devDependencies to dependencies

217b816

To be later used as a peerDependency in the generated package.json It would be better to make this one an optionalPeerDependency but npm currently doesn't support making peerDependencies optional. See: npm/npm#3066

build(npm): dedupe es6-promise dependency

5202d13

IgorMinar force-pushed the peer-deps branch from 5a851ba to 9137314 Compare December 8, 2015 18:47

IgorMinar added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Dec 8, 2015

jelbourn added the zomg_admin: do merge label Dec 8, 2015

IgorMinar closed this in 80b025a Dec 8, 2015

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019

Conversation

IgorMinar commented Dec 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkozlowski-opensource commented Dec 6, 2015

Uh oh!

IgorMinar commented Dec 6, 2015

Uh oh!

IgorMinar commented Dec 6, 2015

Uh oh!

IgorMinar commented Dec 6, 2015

Uh oh!

IgorMinar commented Dec 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkozlowski-opensource commented Dec 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffbcross commented Dec 7, 2015

Uh oh!

IgorMinar commented Dec 7, 2015

Uh oh!

IgorMinar commented Dec 7, 2015

Uh oh!

IgorMinar commented Dec 7, 2015

Uh oh!

pkozlowski-opensource commented Dec 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IgorMinar commented Dec 8, 2015

Uh oh!

IgorMinar commented Dec 8, 2015

Uh oh!

jelbourn commented Dec 8, 2015

Uh oh!

IgorMinar commented Dec 8, 2015

Uh oh!

IgorMinar commented Dec 8, 2015

Uh oh!

jelbourn commented Dec 8, 2015

Uh oh!

angular-automatic-lock-bot bot commented Sep 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants