apm-agent-nodejs icon indicating copy to clipboard operation
apm-agent-nodejs copied to clipboard

drop support for obsolete `hapi` package

Open trentm opened this issue 3 years ago • 5 comments

To be clear, I'm talking about hapi, and not about dropping @hapi/hapi.

Starting with v17.9.0 and v18.2.0 the name changed from 'hapi' to '@hapi/hapi'. hapi (the old one) is deprecated, obsolete, unmaintained and has known major security issues. IIUC it is coming up on two years being so. https://github.com/hapijs/hapi/issues/4114 provides some details. Anything before @hapi/hapi@20 is in this "deprecated, ..." group.

% npm ci
npm WARN deprecated [email protected]: This version contains severe security issues and defects and should not be used! Please upgrade to the latest version of @hapi/hapi or consider a commercial license (https://github.com/hapijs/hapi/issues/4114)
...

There are maintenance and testing costs to continuing to support it. The monstrous .tav.yml block hints at some of the complexity: https://github.com/elastic/apm-agent-nodejs/blob/a289d4428c2c1ac3a57b0767794cb28e928c1da4/.tav.yml#L408-L464

Also, note there are lingering minor issues:

  • https://github.com/npm/cli/issues/2267 is an old unfixed issue with npm install and older versions of hapi (versions that we do currently install and test in our TAV tests)
  • The old 'hapi' packages used npm-shrinkwrap, which results in "extraneous" package entries in package-lock.json for npm versions before v8.6 (the default in node v16). So 'hapi' will be a contributor to noise in our package-lock updates.

open questions

  • Do we drop it now, or do we wait for a major version bump of the agent?

trentm avatar May 12 '22 17:05 trentm

  • which results in "extraneous" package entries in package-lock.json for npm versions before v8.6

I'm wrong about this being a "npm v8.6" difference. Using npm 8.10.0 locally I sometimes get the "extraneous" entries and sometimes not... depending on the exact npm install ... command used. Sigh.

trentm avatar May 12 '22 17:05 trentm

Dependabot package-lock.json updates remove “extraneous” entries. Removing those entries results in npm install (with node v14’s npm v6) failing to install the ‘hapi’ package’s deps and tests fail. I think this is now the main motivator for dropping hapi. In a trade-off between testing old/obsolete 'hapi' vs. using dependabot for security and dep updates, I'll take dependabot.

trentm avatar May 12 '22 18:05 trentm

In the team meeting, @estolfo suggested dropping testing of the obsolete "hapi" package for now, and defer dropping actual support for it until the next major. I think that sounds good. I will work on a PR to try that. If that works, I'll put this issue on the "next-major" milestone.

trentm avatar May 17 '22 22:05 trentm

And just make sure you document that the obsolete hapi package support is untested and deprecated so that users know they are using its instrumentation it at their own risk.

estolfo avatar May 19 '22 08:05 estolfo

And just make sure you document that the obsolete hapi package support is untested and deprecated so that users know they are using its instrumentation it at their own risk.

Yup, that was done both in the changelog (https://github.com/elastic/apm-agent-nodejs/pull/2698/files#diff-cb0601935fcaac3fea29b215282428b964b8b6e1bd5a6b8c68e84d651633c9e3) and supported-technologies doc page (https://github.com/elastic/apm-agent-nodejs/pull/2698/files#diff-d2a92236665ca681fc14a4b85cb60e74bf3d25b42328ca2390fca53156100d25) of #2698

trentm avatar May 19 '22 15:05 trentm

Resolved by https://github.com/elastic/apm-agent-nodejs/pull/3541 on the "dev/4.x" branch, which will be the 4.x active branch soonish.

trentm avatar Aug 03 '23 16:08 trentm