elastic / ecs-logging-nodejs

https://www.elastic.co/guide/en/ecs-logging/nodejs/master/intro.html
Apache License 2.0
68 stars 39 forks source link

bump up fast-json-stringify to stop using deprecated string-similarity #145

Closed ora7598 closed 1 year ago

ora7598 commented 1 year ago

What

Upgrade fast-json-stringify from v2.4.1 to v5.5.0

Why

@elastic/ecs-pino-format depends on @elastic/ecs-helpers, which depends on an old version of fast-json-stringify, which depends on string-similarity which is deprecated.

since v5.0 of fast-json-stringify, it does not depend on string-similarity anymore.

Changeset

cla-checker-service[bot] commented 1 year ago

💚 CLA has been signed

trentm commented 1 year ago

@ora7598 Thanks very much for the PR!

Unfortunately the latest fast-json-stringify uses syntax that only works with node >=v14 and the current ecs-logging-nodejs packages support node >=10. That means we'll have to sit on this PR for a little while until we have a major version bump on these modules that moves the min supported node to >=v14. I hope to get to that, but it won't be very soon.

trentm commented 1 year ago

AFAIK string-similarity is only deprecated because it is no longer maintained, and doesn't have any known security issues against it. If there were security concerns with it, then that would bump the priority of this.

ora7598 commented 1 year ago

Thank you @trentm for the quick response! Can you please share where exactly did you see that fast-json-stringify supports node >= 14? I can't find any evidence of this in npm / its code. Cause when I upgraded it locally, I ran a check for errors and I didn't notice anything suspicious...

trentm commented 1 year ago

Can you please share where exactly did you see that fast-json-stringify supports node >= 14?

% npm install fast-json-stringify
...

% npm ls
a@1.0.0 /Users/trentm/tmp/a
└── fast-json-stringify@5.7.0

% node12
Welcome to Node.js v12.22.12.
Type ".help" for more information.
> require('fast-json-stringify')
/Users/trentm/tmp/a/node_modules/@fastify/deepmerge/index.js:72
  const cloneProtoObject = typeof options?.cloneProtoObject === 'function'
                                          ^

Uncaught SyntaxError: Unexpected token '.'

The @fastify/deepmerge@1.3.0 module used by fast-json-stringify@5.7.0 is using optional chaining syntax that is only available in node v14.0.0 and later.

fast-json-stringify used to have an "engines" in their "package.json" file to specify the min supported node version, but it was removed in https://github.com/fastify/fast-json-stringify/pull/463 because, I guess, "it is hard to manage".

ora7598 commented 1 year ago

Thanks @trentm. After a quick check, I found out that fast-json-stringify@5.0.6 isn't using deepmerge@1.3.0, and I also verified that it works with node v10. Any chance we can at least upgrade the package to this version (5.0.6)? Just to prevent it from using that deprecated package.

ora7598 commented 1 year ago

I modified my commit and updated to version 5.0.6.

trentm commented 1 year ago

Sorry for the delays in responding. Yes we can probably lock to fast-json-stringify@5.0.6 -- though it feels a little bit fast and loose given fast-json-stringify basically implicitly only supporting node >=14.x for the 5.x series. :)

I'll commit the suggested change and get tests running.

trentm commented 1 year ago

I spoke too soon. fastify-json-stringify@5 uses crypto.randomUUID() (https://nodejs.org/api/crypto.html#cryptorandomuuidoptions) which was "Added in: v15.6.0, v14.17.0". Sigh.

(Aside: Is there a particular concern with the usage of the deprecated string-similarity beyond the deprecation warning on install? I understand that isn't nothing -- the deprecation message on install of downstream apps is bad optics for users.)

I'm leaning more towards needing to do a major version bump of the ecs-logging-nodejs libraries that drops support for anything less than v14.17.0. I'm a little piqued at the tail (fast-json-stringify) wagging the dog (these libs) here.

ora7598 commented 1 year ago

Thanks for checking @trentm. No there is no concern with usage of that deprecated package beyond the warning :) Guess we will have to wait for a major version bump. Thanks again for your willingness to help!

trentm commented 1 year ago

This will be resolved via https://github.com/elastic/ecs-logging-nodejs/pull/155