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

Make use of new safe-stable-stringify 2.x features #134

Merged
merged 1 commit into from
Feb 12, 2022

Conversation

mastermatt
Copy link
Contributor

https://github.com/BridgeAR/safe-stable-stringify/releases

  • Allow opts to configure the lib.
  • Update types for new opts.
  • Add tests to fill in coverage for json.js.

NB. The Buffer.toString('base64') conversion was removed. Buffer implements toJSON so this block of code was not being executed anyway, as the replacer is called with the result of toJSON. Base64 was never being returned.
https://nodejs.org/api/buffer.html#buftojson

@DABH
Copy link
Contributor

DABH commented Feb 12, 2022

@mastermatt I'm taking a look at this now. I just merged a dependabot PR that upgraded to 2.x so could you please rebase or fix merge conflicts here and I can look at the changes you have besides the version bump? Thanks!

@mastermatt mastermatt changed the title Upgrade safe-stable-stringify lib to 2.x Make use of new safe-stable-stringify 2.x features Feb 12, 2022
@mastermatt
Copy link
Contributor Author

@DABH done.

This commit originally included an upgrade of the lib itself from 1.x to 2.x.
After a Dependabot PR was merged in to do the upgrade, this commit was rebased to only include the changes that take advantage of the new features.
https://github.com/BridgeAR/safe-stable-stringify/releases

----

- Allow `opts` to configure the lib.
- Update types for new opts.
- Add tests to fill in coverage for json.js.

NB. The `Buffer.toString('base64')` conversion was removed. Buffer implements `toJSON` so this block of code was not being executed anyway, as the replacer is called with the result of `toJSON`. Base64 was never being returned.
https://nodejs.org/api/buffer.html#buftojson
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.

LGTM, thank you for adding tests!

@DABH DABH merged commit e3a8000 into winstonjs:master Feb 12, 2022
@mastermatt mastermatt deleted the upgrade-safe-stable-stringify branch February 14, 2022 14:28
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.

2 participants