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

Persist original JSON response #60

Closed
1ec5 opened this issue Jul 24, 2016 · 2 comments
Closed

Persist original JSON response #60

1ec5 opened this issue Jul 24, 2016 · 2 comments
Labels
improvement Improvement for an existing feature. performance

Comments

@1ec5
Copy link
Contributor

1ec5 commented Jul 24, 2016

An application may want to add its own request/response logging functionality. Currently, if it uses the built-in URL request functionality, it has to transform Route objects and so forth into a string-convertible format. We should add the original serialized JSON to the completion handler just in case the application needs it.

/cc @willwhite

@1ec5 1ec5 added the improvement Improvement for an existing feature. label Jul 24, 2016
@1ec5 1ec5 added this to the v1.0 milestone Dec 28, 2019
@1ec5
Copy link
Contributor Author

1ec5 commented Dec 28, 2019

We could consider storing the original JSON response in the RouteResponse class that was made public in #393 and would be documented as part of #391.

Storing the original JSON would incur some additional memory usage, but it would avoid the potentially expensive roundtripping from JSON to Swift model objects back to JSON that the navigation SDK has to perform for MapboxNavigationNative. That roundtripping has also proven fairly brittle, because MapboxNavigationNative is a lot stricter than this library has historically been about the JSON that it accepts.

@1ec5 1ec5 changed the title Add hook for response logging Persist original JSON response Dec 28, 2019
@1ec5 1ec5 removed this from the v1.0.0 milestone Apr 14, 2020
@1ec5
Copy link
Contributor Author

1ec5 commented Dec 14, 2021

Currently, if it uses the built-in URL request functionality, it has to transform Route objects and so forth into a string-convertible format.

This issue was originally written against a version of the library that supported Objective-C and didn’t use Codable, so round-tripping was a laborious, error-prone affair. Migrating to Codable eliminated a large class of errors and made us feel much more confident about relying on round-tripping.

Storing the original JSON would incur some additional memory usage, but it would avoid the potentially expensive roundtripping from JSON to Swift model objects back to JSON that the navigation SDK has to perform for MapboxNavigationNative.

JSON conversions no longer figure as large in performance profiles as they did when we relied on JSONSerialization directly, and the performance problems we initially experienced when investigating Codable have not turned out to be a noticeable problem in practice.

That roundtripping has also proven fairly brittle, because MapboxNavigationNative is a lot stricter than this library has historically been about the JSON that it accepts.

#637 proposes storing each JSON object’s unrecognized keys in a loosely typed property and round-tripping it when encoding back to JSON. That approach effectively solves this problem, because clients such as MapboxNavigationNative only need the JSON to be equivalent in value; any differences in whitespace formatting and key order shouldn’t matter.

@1ec5 1ec5 closed this as completed Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement for an existing feature. performance
Projects
None yet
Development

No branches or pull requests

1 participant