-
Notifications
You must be signed in to change notification settings - Fork 905
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
[2.x][Console] Add support for JSON with long numerals (#4562) #5095
Conversation
8431077
to
32f1304
Compare
Codecov Report
@@ Coverage Diff @@
## 2.x #5095 +/- ##
==========================================
+ Coverage 66.48% 66.54% +0.06%
==========================================
Files 3402 3403 +1
Lines 64982 65084 +102
Branches 10374 10402 +28
==========================================
+ Hits 43201 43310 +109
+ Misses 19218 19207 -11
- Partials 2563 2567 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
a15fd54
to
b2debc6
Compare
396d2c8
to
8ee7ff5
Compare
…#4562)(opensearch-project#5130) Also: * Add support for parsing and stringifying JSON with long numerals into `@osd/std` * Upgrade to `@opensearch/[email protected]` which supports long numerals * Add support for long numerals to `http/fetch` * Improve testing of clients with long numerals support Signed-off-by: Miki <[email protected]>
8ee7ff5
to
5ba4587
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting harder to support dependency management BWC 😢. But thanks Miki!
Trying to think of a way to handle this in the future without doing it like this but every way seems like over engineering.
Added the 2.11 label |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no objections, but just trying to wrap my head around our usage of both client and client next.
? new ClientNext({ ...clientOptions, enableLongNumeralSupport: true } as ClientOptionsNext) | ||
: new Client(clientOptions as ClientOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to avoid ClientNext
by default? Is there a breaking change failure case that's supported but incompatible with long numeral support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The simplest breaking change is introduced by differing typing as a result of changes to ClientOptions
. It is the same reason that we couldn't replace the JS client v1 with v2 in 2.x earlier.
I propose more often major bumps; not every other month kind of thing but perhaps a once yearly one to allow us to introduce potentially breaking changes that are needed to provide users a better piece of software. |
Because of opensearch-project/OpenSearch-Dashboards#5095, the URL parsing is handled differently on main branch and 2.x branch. Therefore, this cypress test on 2.x branch needs to be modified. Signed-off-by: Qingyang(Abby) Hu <[email protected]>
Because of opensearch-project/OpenSearch-Dashboards#5095, the URL parsing is handled differently on main branch and 2.x branch. Therefore, this cypress test on 2.x branch needs to be modified. Signed-off-by: Qingyang(Abby) Hu <[email protected]> (cherry picked from commit 518ce68)
Because of opensearch-project/OpenSearch-Dashboards#5095, the URL parsing is handled differently on main branch and 2.x branch. Therefore, this cypress test on 2.x branch needs to be modified. Signed-off-by: Qingyang(Abby) Hu <[email protected]> (cherry picked from commit 518ce68) Co-authored-by: Qingyang(Abby) Hu <[email protected]>
Backport 0709333 from #4562
Note: This is not just a simple cherry-pick; the logic had to be modified a lot to accommodate simultaneous use of the 2 versions of the JS client.
[Console] Add support for JSON with long numerals (#4562)
Also:
@osd/std
@opensearch/[email protected]
which supports long numeralshttp/fetch
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr