search

jochen-schweizer / express-prom-bundle

express middleware with standard prometheus metrics in one bundle
MIT License
303 stars 67 forks source link

Intentional breaking changes? #104

Closed wickedest closed 2 years ago

wickedest commented 2 years ago

Is it intentional to accept all breaking changes from prom-client as a peerDependency? In the package.json,

  "peerDependencies": {
    "prom-client": ">=12.0.0"
  },

My build started to fail a because the following new metrics were introduced in prom-client 14.1.0.

nodejs_active_resources
nodejs_active_resources_total

Because of the peerDependency, prom-client v14 was installed (maybe v13 that was specified as a devDependency was intented).

At any rate, prom-client could introduce breaking changes that will cause express-prom-bundle to fail, and maybe already has in 14 in a way that I am not testing.

disjunction commented 2 years ago

@wickedest It was a compromise to allow people using both 12th and 13th versions of prom-client. The code has a workaround to hande those both versions. We can add the support for 14th now.

But why does adding a couple of metrics break the build in your case?

wickedest commented 2 years ago

@disjunction, sorry, I was a little unclear. I did not hit a breaking change. My test was checking the expected metrics, and they changed unexpectedly. However, there could be breaking changes in prom-client 14 that affect express-prom-bundle that my tests didn't pick up. The point is, because of the peerDepenency ">= 12", npm is going to automatically install every breaking change from prom-client, whether or not express-prom-bundle works with them.

It may be a good idea to limit the peerDependency to "^14" (assuming it's compatible), and minor bump. That way, when 15 rolls around, it may not break things unexpectedly.

disjunction commented 2 years ago

But the downside is that if there is NO breaking change and people use the latest prom-client, then they suddenly can't use the prom-bundle. The project exists for 4 years and I can remember just one case (v13) when a breaking change was introduced, otherwise people were simply upgrading their prom-client whenever they wanted and the bundle was still working for them.

This is why I decided at the time to be less restrictive and just adjust the library whenever the true problem occurs.

BTW peer dependencies are NOT installed automatically afaik, thus it's a conscious decision every time.

wickedest commented 2 years ago

BTW peer dependencies are NOT installed automatically afaik, thus it's a conscious decision every time.

That is not longer true since npm 7.

In npm versions 3 through 6, peerDependencies were not automatically installed, and would raise a warning if an invalid version of the peer dependency was found in the tree. As of npm v7, peerDependencies are installed by default.

Trying to install another plugin with a conflicting requirement may cause an error if the tree cannot be resolved correctly. For this reason, make sure your plugin requirement is as broad as possible, and not to lock it down to specific patch versions.

So, it's no longer a suggestion, it's a demand, and npm will install one or multiple versions of prom-client if necessary to satisfy express-prom-bundle's peerDependency.

But I made a mistake here and sorry to waste your time. I'm not entirely correct about this:

npm is going to automatically install every breaking change from prom-client

If a project does not explicitly depend on prom-client, then npm will install the latest (i.e. v14). But that's probably never going to happen in this case, and your peerDependency is fine.

My project does explicitly depend on prom-client v14 (but I didn't realize that). So, in my case, it installed v14 exactly as I told it to. Sorry about (my) confusion!