Skip to content

feat(NODE-4406): direct EJSON stringification for UUIDs #511

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

Closed

Conversation

aditi-khare-mongoDB
Copy link
Contributor

@aditi-khare-mongoDB aditi-khare-mongoDB commented Aug 2, 2022

Description

What is changing?

ESJON now supports direct stringification of UUID, without needing to call UUID.toBinary() in the argument.

Is there new documentation needed for these changes?

Yes

What is the motivation for this change?

EJSON.stringify({ u: new UUID() }); used to result in an error, but now it is a valid operation

Double check the following

  • Ran npm run lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@aditi-khare-mongoDB aditi-khare-mongoDB changed the title WIP! feat(NODE-4406): direct EJSON stringification for UUIDs feat(NODE-4406): direct EJSON stringification for UUIDs Aug 2, 2022
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title feat(NODE-4406): direct EJSON stringification for UUIDs WIP feat(NODE-4406): direct EJSON stringification for UUIDs Aug 2, 2022
Copy link
Contributor

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Looks good to me!

I'd be curious to hear from the team on what the line of thought behind not making UUID a subclass of Binary – when the UUID class was first introduced, I thought that was done specifically because it was not intended to work with bson/EJSON serialization/deserialization, but since that's being changed, I'm wondering why UUID is not implemented that way.

@aditi-khare-mongoDB aditi-khare-mongoDB changed the title WIP feat(NODE-4406): direct EJSON stringification for UUIDs feat(NODE-4406): direct EJSON stringification for UUIDs Aug 2, 2022
@nbbeeken
Copy link
Contributor

nbbeeken commented Aug 3, 2022

Good idea, I suppose if we subclassed we would inherit all the EJSON logic both to and from 🤔
I'll bring this up in the team channel as an alternative approach, I think it's the right idea

durran
durran previously approved these changes Aug 8, 2022
@durran durran dismissed their stale review August 8, 2022 17:54

Subclass work

@durran
Copy link
Member

durran commented Aug 9, 2022

Closed in favor of #512

@durran durran closed this Aug 9, 2022
@durran durran deleted the NODE-4406-direct-EJSON-stringification branch August 9, 2022 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants