Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from

Conversation

ora7598
Copy link

@ora7598 ora7598 commented May 11, 2023

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
Copy link

cla-checker-service bot commented May 11, 2023

💚 CLA has been signed

@github-actions github-actions bot added agent-nodejs Make available for APM Agents project planning. community triage labels May 11, 2023
@ora7598 ora7598 marked this pull request as ready for review May 11, 2023 12:15
@trentm
Copy link
Member

trentm commented May 11, 2023

@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 trentm removed the triage label May 11, 2023
@trentm
Copy link
Member

trentm commented May 11, 2023

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
Copy link
Author

ora7598 commented May 14, 2023

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
Copy link
Member

trentm commented May 15, 2023

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 fastify/fast-json-stringify#463 because, I guess, "it is hard to manage".

@ora7598
Copy link
Author

ora7598 commented May 16, 2023

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
Copy link
Author

ora7598 commented May 18, 2023

I modified my commit and updated to version 5.0.6.

helpers/package.json Outdated Show resolved Hide resolved
@trentm
Copy link
Member

trentm commented May 24, 2023

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
Copy link
Member

trentm commented May 24, 2023

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
Copy link
Author

ora7598 commented May 28, 2023

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 added a commit that referenced this pull request Oct 18, 2023
This drops the schema-based fast-json-stringify usage, which could
potentially be faster, but will throw on circular references. The
safer serialization is more appropriate for logging libs. Note that
currently both pino and winston themselves use safe-stable-stringify.

Closes: #104
Closes: #145
@trentm
Copy link
Member

trentm commented Oct 18, 2023

This will be resolved via #155

@trentm trentm closed this in #155 Oct 24, 2023
@trentm trentm closed this in efb11b7 Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning. community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants