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

Use json-big to parse main query response #24

Merged
merged 5 commits into from
Jun 27, 2024
Merged

Conversation

albireox
Copy link
Member

Related to sdss/valis#23.

This PR modifies how the response JSON from the API query for the main search page is parsed. Normally big integers are not preserved when parsin JSON; instead they are cast to float and then back to integer, which causes rounding errors. Using json-bigint solves this issue.

See here for more details. I haven't confirmed if there are other places in which this transformation need to be done.

Copy link
Contributor

@havok2063 havok2063 left a comment

Choose a reason for hiding this comment

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

So it turns out I encountered the exact same issue, and solved in the Target.vue code. I used a different package, json-big, but it looks the same as json-bigint. In fact json-big appears to a fork of json-bigint and I don't see any major differences between the two. I would suggest we consolidate to a single package. I'm happy to switch to json-bigint, as that seems to be the more updated and main package.

It would also be good if we could apply the axios tweak to transformResponse globally for all uses. I'm not sure what the best way is to do that though. If that's not straightforward then we can leave the separate tweaks for now.

src/views/Search.vue Outdated Show resolved Hide resolved
src/views/Search.vue Outdated Show resolved Hide resolved
@albireox albireox changed the title Use json-bigint to parse main query response Use json-big to parse main query response Jun 26, 2024
@albireox
Copy link
Member Author

I've kept json-big since I don't think there are big (pun intended) differences.

Have a look at the new commits I made. I've moved the configuration of the axios instance to an api.ts file (the name and location can change) where the JSON bigint handling is done. Then you can use that instance to do API requests and it's not necessary to do the same bigint configuration each time. I used a 5 second timeout which may be insufficient in some cases.

I made this change for Target and Search but there may be other places that need it. I did a bit of testing and it seems to work but it would be good if you can test independently. The docs for the axios instance are here

https://axios-http.com/docs/instance

Copy link
Contributor

@havok2063 havok2063 left a comment

Choose a reason for hiding this comment

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

Your changes seem reasonable to me. I made a few comments. We are using axios in a bunch of places. Should we replace them everywhere? The only places where it might make sense, but really probably doesn't matter, are the ConeSearch and TargetResolver components. But at least we wouldn't need to always specify the base url.

Dare I suggest you try to write some unit tests for these changes? Feel free to say no. I haven't been consistent about it but I occasionally write some. You can take a look at the components/__test__ directory.

src/api.ts Outdated Show resolved Hide resolved
src/views/Target.vue Outdated Show resolved Hide resolved
src/views/Target.vue Outdated Show resolved Hide resolved
@havok2063
Copy link
Contributor

Actually, to keep this PR focused on the main query, we don't need to replace all instances of axios. We can do that in a later PR. I'll create a new issue for it. I'll need to add global custom headers for the authentication to this config anyways

@albireox
Copy link
Member Author

I agree, let's do that as a different PR, but I'll add myself to #25. And I'll write some tests; I'm actually interested in learning how to test JS and I see you have some examples there.

@havok2063
Copy link
Contributor

Looks good to me!

@albireox albireox merged commit 1d1be1c into main Jun 27, 2024
1 check passed
@albireox albireox deleted the albireox/json-bigint branch June 27, 2024 16:10
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