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.

Sign up for GitHub

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

Jump to bottom

Make handling of long numerals an option that is disabled by default #557

Merged
merged 1 commit into from
Jul 14, 2023
Merged

Make handling of long numerals an option that is disabled by default #557

merged 1 commit into from
Jul 14, 2023

Conversation

AMoo-Miki
Copy link
Collaborator

Description

This change adds a config param named enableLongNumeralSupport which is false by default and users who wish to deal with precise long numerals can enable it.

This change needs to be added to the docs

Check List

  • New functionality includes testing.
    • All tests pass
  • Linter check was successfull - yarn run lint doesn't show any errors
  • Commits are signed per the DCO using --signoff
  • Changelog was updated.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@AMoo-Miki AMoo-Miki force-pushed the allow-disable-bigint branch from 30467b0 to 2629b6b Compare July 13, 2023 18:27
dblock
dblock requested changes Jul 13, 2023
View reviewed changes
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

What's the reason for this? What are the downsides of always serializing long numerals correctly?

What do you think about calling this serializeLongNumerals?

This PR needs an update to USERS_GUIDE, too, please.

@AMoo-Miki AMoo-Miki force-pushed the allow-disable-bigint branch from 2629b6b to df14ac3 Compare July 13, 2023 22:56
@AMoo-Miki
Make handling of long numerals an option that is disabled by default
c6d1642 
Also:
* Strengthen the tests
* update USER_GUIDE.md

Signed-off-by: Miki <miki@amazon.com>
@AMoo-Miki AMoo-Miki force-pushed the allow-disable-bigint branch from df14ac3 to c6d1642 Compare July 13, 2023 23:16
@AMoo-Miki
Copy link
Collaborator Author

What's the reason for this? What are the downsides of always serializing long numerals correctly?

It is possible that users have adapted their logic to having imprecise numbers and are not ready to accept BigInts. One could argue that what we release in 2.3.0 was an enhancement but some could also argue that by successfully serializing BigInts (and not throwing) and deserializing long numerals into BigInts, we broke status quo and went against the universally known limitation of JSON.parse and JSON.stringify that people expect; one could argue that doing this by default is a breaking change.

What do you think about calling this serializeLongNumerals?

We could surely name it serializeLongNumerals for those who will be using it by importing the Serializer but the Client needs a switch to use or not use this. Instead of the Client and Helpers each having conditions to use serialize or serializeLongNumerals, in this PR, I handle the condition in only one place which is inside serialize.

This PR needs an update to USERS_GUIDE, too, please.

Done.

@AMoo-Miki AMoo-Miki requested a review from dblock July 14, 2023 05:49
@dblock
Copy link
Member

dblock commented Jul 14, 2023

Change LGTM. Since this is breaking what do we want to do about semver? @wbeckler

@dblock dblock merged commit 08069bc into opensearch-project:main Jul 14, 2023
@dblock dblock mentioned this pull request Jul 14, 2023
Closed
5 tasks
@nhtruong
Copy link
Collaborator

nhtruong commented Jul 14, 2023 •
edited
Loading

looks like we need a 2.3.1 release

@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Jul 14, 2023
Merged
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 14, 2023
@github-actions
Make handling of long numerals an option that is disabled by default (#…
c661ce0 
…557)

Also:
* Strengthen the tests
* update USER_GUIDE.md

Signed-off-by: Miki <miki@amazon.com>
(cherry picked from commit 08069bc)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@wbeckler
Copy link

Can someone ELI5 why this is a breaking change?

@dblock
Copy link
Member

dblock commented Jul 17, 2023

Can someone ELI5 why this is a breaking change?

Unless I misunderstood something, we released 2.3.0 where the handling of long numerals is enabled by default, and 2.3.1 would disable it by default.

dblock pushed a commit that referenced this pull request Jul 17, 2023
@opensearch-trigger-bot @github-actions
Make handling of long numerals an option that is disabled by default (#…
198d3a2 
…557) (#561)

Also:
* Strengthen the tests
* update USER_GUIDE.md


(cherry picked from commit 08069bc)

Signed-off-by: Miki <miki@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
nhtruong added a commit that referenced this pull request Jul 18, 2023
@nhtruong @AMoo-Miki
@opensearch-trigger-bot @github-actions
Restore 2.3 (#568)
f9a2fe1 
* Add upgrading NPM to all workflows running older Node.js versions (#545) (#553)

Also:
* Add compatibility checks for Node.js v18
* Bump `actions/setup-node` to v3

Signed-off-by: Miki <miki@amazon.com>

* Add serialization and deserialization of numerals larger than `Number.MAX_SAFE_INTEGER` (#544) (#554)

Signed-off-by: Miki <miki@amazon.com>

* Version Bump: 2.3.0 (#546) (#555)

Signed-off-by: Theo Truong <theotr@amazon.com>

* Make handling of long numerals an option that is disabled by default (#557) (#561)

Also:
* Strengthen the tests
* update USER_GUIDE.md


(cherry picked from commit 08069bc)

Signed-off-by: Miki <miki@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Version Bump: 2.3.1

Signed-off-by: Theo Truong <theotr@amazon.com>

---------

Signed-off-by: Miki <miki@amazon.com>
Signed-off-by: Theo Truong <theotr@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Miki <miki@amazon.com>
Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock approved these changes

@ananzh ananzh Awaiting requested review from ananzh ananzh is a code owner

@kavilla kavilla Awaiting requested review from kavilla kavilla is a code owner

@seanneumann seanneumann Awaiting requested review from seanneumann

@VachaShah VachaShah Awaiting requested review from VachaShah VachaShah is a code owner

@nhtruong nhtruong Awaiting requested review from nhtruong nhtruong is a code owner

@harshavamsi harshavamsi Awaiting requested review from harshavamsi harshavamsi is a code owner

@timursaurus timursaurus Awaiting requested review from timursaurus timursaurus is a code owner

Assignees
No one assigned
Labels
backport 2.3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AMoo-Miki @dblock @nhtruong @wbeckler