Deps correctness #505

mikhailrojo · 2020-05-26T15:24:35Z

I moved libraries which are required in code in deps from devDeps and add commas to readme.
Closes #491

jcchavezs · 2020-05-26T17:10:39Z

packages/zipkin-transport-http/package.json

    "zipkin": "^0.21.0"
  },
  "devDependencies": {
    "body-parser": "^1.15.2",
+    "express": "^4.17.1",


I am curious about this one, I know this is required in the test but how come tests don't complain?

My guess is as it is lerna, expressjs was coming from the global package.json

deleted express as it is in global package json
03c84c6

jcchavezs · 2020-05-26T17:10:49Z

packages/zipkin-instrumentation-request-promise/package.json

@@ -16,16 +16,16 @@
    "@types/request": "^2.0.8"
  },
  "dependencies": {
+    "asap": "^2.0.6",


👍 I think using 2.0.4 is enough https://github.com/kriskowal/asap/blob/master/CHANGES.md

Jose, I set the version 03c84c6
but what is the point of fixing patch version?

Sorry I meant ^2.0.4 but ^2.0.6 is a patch so probably this is good enough 👍

jcchavezs · 2020-05-27T01:39:24Z

packages/zipkin-transport-http/package.json

@@ -14,12 +14,13 @@
  "license": "Apache-2.0",
  "repository": "https://github.com/openzipkin/zipkin-js",
  "dependencies": {
+    "node-fetch": "^2.6.0",


I am kind of concerned about this version constraint. I checked node-fetch api and nothing stop us to use version 1.4.0 for example since it support all the options we use. In the other hand, version 3.x is going to break us. I feel like we could support ^1.4 and ^2.0? What do you think. I don't want us to force users to upgrade their node-fetch because of zipkin.

why do you think this package uses 1.4.0 ? I moved this package from version 2.3 to 2.6
you suggest to fix it here as ^2.0.0?

why do you think this package uses 1.4.0 ? I moved this package from version 2.3 to 2.6
you suggest to fix it here as ^2.0.0?

I think you could specify it as >=2.0.0 < 3.0.0 (could also be that simply 2.x expresses the same thing but don't quote me on that, I've slipped up with semver constraints before)

Could also be >=1.4.0 < 3.0.0 to include the 1.4.0 version as @jcchavezs mentioned

Did I get it right that ^2.0.0 will be fine for everyone?

Did I get it right that ^2.0.0 will be fine for everyone?

Would be fine by me personally because I don't use old node-fetch (v1.4.x) at all.

Yeah, my main concern are those using 1.4.x already that want to use zipkin-js, will that force them to use node-fetch 2.x then? In the other hand I think that since this is not a breaking change because we are just adding the dependency it is fine to keep ^2.0.0. Any thoughts @adriancole?

Yeah, my main concern are those using 1.4.x already that want to use zipkin-js, will that force them to use node-fetch 2.x then?
In the other hand I think that since this is not a breaking change because we are just adding the dependency it is fine to keep ^2.0.0. Any thoughts @adriancole?

If they want to upgrade to the newer version of zipkin-js, then yes, they will be forced to update node-fetch from 1.4.x to at least 2.x.
So, in this sense, this is a breaking change and should be accompanied by a major version bump on the version of zipkin-js itself.

Basically it's like this: If you previously did not restrict a peer dependency version, but now you do, then it is a breaking change for those who are suddenly out of compliance with their dependency version.

If it were released as a minor/patch version bump then it might break builds for some, if they had minor/patch version auto upgrade specified in their zipkin-js version in the package.json dependencies section. This is how builds of projects with npm dependency management can break overnight seemingly all by themselves without anyone changing anything in the source code.

So, I'd say it needs a major release.
That way people who are upgrading their zipkin-js will know to "expect trouble" and those who specified to only auto-update patch/minor versions won't get a sudden and unexpected breakage.

People who specified to auto-upgrade even major versions and use node-fetch 1.4.x will still have their build break overnight, but they chose to live on the bleeding edge so there's nothing you can do about that.

Thanks for the explanation @petermetz, if that is the case then let's do >=1.4.0 < 3.0.0 so we don't break anyone. We are not likely doing a major release for this change.

jcchavezs · 2020-05-27T01:49:03Z

packages/zipkin-instrumentation-request-promise/package.json

@@ -16,16 +16,16 @@
    "@types/request": "^2.0.8"
  },
  "dependencies": {
+    "asap": "^2.0.6",
+    "request": "^2.88.2",
+    "request-promise": "^4.2.5",


I am not sure about this one. It feels like CLS is not supported in v4: https://github.com/request/request-promise#migration-from-v3-to-v4

Do you have any idea?

I moved it from devDeps v 4.2.2 to deps 4.2.5
where is v3?

Yeah, right. Ignore this one.

jcchavezs · 2020-05-27T01:50:30Z

Thanks for this. I added some comments.

I know you are just moving devDeps into deps but that means we will impact user's code and hence it is worth taking the time to look at the deps we are aiming to introduce to our users.

mikhailrojo

I opened #506 request to improve how deps are organized- may be this PR should be delayed until that one?

mikhailrojo · 2020-05-27T07:42:03Z

packages/zipkin-transport-http/package.json

@@ -14,12 +14,13 @@
  "license": "Apache-2.0",
  "repository": "https://github.com/openzipkin/zipkin-js",
  "dependencies": {
+    "node-fetch": "^2.6.0",


why do you think this package uses 1.4.0 ? I moved this package from version 2.3 to 2.6
you suggest to fix it here as ^2.0.0?

mikhailrojo · 2020-05-27T07:43:41Z

packages/zipkin-instrumentation-request-promise/package.json

@@ -16,16 +16,16 @@
    "@types/request": "^2.0.8"
  },
  "dependencies": {
+    "asap": "^2.0.6",
+    "request": "^2.88.2",
+    "request-promise": "^4.2.5",


I moved it from devDeps v 4.2.2 to deps 4.2.5
where is v3?

jcchavezs · 2020-05-28T20:01:01Z

Once this one https://github.com/openzipkin/zipkin-js/pull/505/files#r432086456 is sorted we can merge it and do a release.

… Deps-correctness � Conflicts: � yarn.lock

petermetz

LGTM

jcchavezs · 2020-06-04T19:40:14Z

Thanks a lot @mikhailrojo !

Михаил М. Степанов added 2 commits May 26, 2020 18:14

Moved devDeps to deps

7d48366

Improved deps in transport-http

7b35057

mikhailrojo mentioned this pull request May 26, 2020

Libraries as asap and node-fetch are used but they are not mentioned in deps #491

Closed

jcchavezs reviewed May 26, 2020

View reviewed changes

jcchavezs mentioned this pull request May 26, 2020

feat: adds support for cls-hooked. #476

Merged

jcchavezs reviewed May 27, 2020

View reviewed changes

Deleted express as it is in global package and set version of asap

03c84c6

mikhailrojo commented May 27, 2020

View reviewed changes

Михаил М. Степанов added 4 commits June 4, 2020 13:55

Moved node-fetch to peers to avoid breaking changes

3448533

Reverted yarn.lock

5151c8c

Merge branch 'master' of https://github.com/openzipkin/zipkin-js into…

96145e6

… Deps-correctness � Conflicts: � yarn.lock

solved conflict and yarn installed

a51ec31

petermetz approved these changes Jun 4, 2020

View reviewed changes

jcchavezs merged commit edbed6b into openzipkin:master Jun 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deps correctness #505

Deps correctness #505

mikhailrojo commented May 26, 2020 •

edited by jcchavezs

Loading

jcchavezs May 26, 2020 •

edited

Loading

mikhailrojo May 26, 2020

mikhailrojo May 27, 2020 •

edited

Loading

jcchavezs May 26, 2020 •

edited

Loading

mikhailrojo May 27, 2020

jcchavezs May 28, 2020 •

edited

Loading

jcchavezs May 27, 2020

mikhailrojo May 27, 2020

petermetz May 27, 2020

mikhailrojo May 28, 2020

petermetz May 28, 2020

jcchavezs May 28, 2020 •

edited

Loading

petermetz May 28, 2020

jcchavezs May 28, 2020

mikhailrojo Jun 4, 2020

jcchavezs May 27, 2020

mikhailrojo May 27, 2020

jcchavezs May 28, 2020

jcchavezs commented May 27, 2020

mikhailrojo left a comment

mikhailrojo May 27, 2020

mikhailrojo May 27, 2020

jcchavezs commented May 28, 2020

petermetz left a comment

jcchavezs commented Jun 4, 2020

Deps correctness #505

Deps correctness #505

Conversation

mikhailrojo commented May 26, 2020 • edited by jcchavezs Loading

jcchavezs May 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikhailrojo May 27, 2020 • edited Loading

Choose a reason for hiding this comment

jcchavezs May 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs May 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs May 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs commented May 27, 2020

mikhailrojo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs commented May 28, 2020

petermetz left a comment

Choose a reason for hiding this comment

jcchavezs commented Jun 4, 2020

mikhailrojo commented May 26, 2020 •

edited by jcchavezs

Loading

jcchavezs May 26, 2020 •

edited

Loading

mikhailrojo May 27, 2020 •

edited

Loading

jcchavezs May 26, 2020 •

edited

Loading

jcchavezs May 28, 2020 •

edited

Loading

jcchavezs May 28, 2020 •

edited

Loading