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

Replace JSON stringify library "fast-safe-stringify" by "safe-stable-stringify" #98

Merged

Conversation

markablov
Copy link
Contributor

Reasons for replacements:

  1. It's faster (https://github.com/BridgeAR/safe-stable-stringify#performance--benchmarks)
  2. It has less bugs (i also met same bug - Infinite loop when called on a Sequelize object davidmarkclements/fast-safe-stringify#46)

From internal tests "safe-stable-stringify" is working faster and still produces correct output.

Only single disadvantage - you can't have randomness in property order, it's always stable.

markablov and others added 3 commits December 18, 2019 19:19
Also have removed "stable" option for json formatter, because new
JSON stringification library always produces stable results with better
performance
Copy link
Contributor

@DABH DABH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a reasonable change to me. Thanks!

@DABH DABH merged commit b91bfa5 into winstonjs:master Jul 11, 2020
@vladholubiev
Copy link

@DABH do you mind publishing a new version of logform package which includes this fix?

@markablov markablov deleted the feat/switch-to-better-json-stringifier branch March 3, 2021 03:17
vdemonchy added a commit to Spendesk/winston that referenced this pull request Aug 6, 2021
@vdemonchy
Copy link

@DABH Hello, I second @vladgolubev comment, would you please consider releasing a new version of the package with the json lib switch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants