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

Add JSON path tracking to Encode/Decode #780

Closed
aleclarson opened this issue Mar 9, 2024 · 2 comments · Fixed by #791
Closed

Add JSON path tracking to Encode/Decode #780

aleclarson opened this issue Mar 9, 2024 · 2 comments · Fixed by #791
Labels
future Candidate future functionality

Comments

@aleclarson
Copy link

aleclarson commented Mar 9, 2024

When using Transform(…).Decode(…).Encode(…), it would be great if JSON paths were forwarded through to the Decode/Encode callbacks. Then I would be able to thread the JSON path through to any nested Value.Encode/Decode calls.

Here's an example of what I'm doing:

const JsonTransform = <T extends TSchema>(schema: T, options?: ArrayOptions) =>
  Transform(t.String(options))
    .Decode(value => {
      return Value.Decode(schema, JSON.parse(value))
    })
    .Encode(value => {
      return JSON.stringify(Value.Encode(schema, value))
    })

types.GinkgoObject = <T extends TProperties>(
  properties: T,
  options?: SchemaOptions
) => JsonTransform(t.Object(properties, options))

types.GinkgoArray = <T extends TSchema>(schema: T, options?: ArrayOptions) =>
  JsonTransform(t.Array(schema, options))

These types are being used for JS<->SQLite marshalling.

The Problem

When a nested encode/decode fails, the error doesn't have the full JSON path in it, making it hard to debug.

aleclarson added a commit to alloc/typebox that referenced this issue Mar 9, 2024
Use a stack for Encode/Decode to detect when it‘s a nested call. If nested, the `Errors` function is **not** called, since the produced error would only be a fraction of the full JSON path. This change is beneficial for certain `Transform` usage, as described in sinclairzx81#780.

Closes sinclairzx81#780
@aleclarson
Copy link
Author

Now that I better understand the internals, I'd say that #781 is enough to close this issue out, since all I wanted was the full JSON path to be attached to nested Encode/Decode errors.

@sinclairzx81
Copy link
Owner

@aleclarson Hey, Let me give this some thought on how best to proceed here.

I do like this idea (especially for debugging), but it's going to take some refactoring to propagate the navigable path through the current Encode/Decode transform logic. Currently the best example of how TB achieves this is via the Errors logic, so will likely want to replicate a similar pattern for Transforms. This will also need some tests written.

I think between this and the other PR's submitted for Transforms, an additional general review of the way TypeBox currently handles Transform error handling might be good (I'm very much open to ways to improve things there). I think mostly, getting some concrete use cases (code examples) might be a good way to communicate where things could improve. In the mean time, I've left some fairly detailed notes on the PR's you've submitted (these will need a follow up review from you), Will spend some time reviewing follow ups a bit later on this week.

Also hey, thanks for submitting these PR's. It's great to get some visibility on the current TB Transform logic (as these have been quite difficult to get right). All community inputs to help improve this aspect of TB are very much appreciated :)

Will pick things up later this week.
Cheers!
S

@sinclairzx81 sinclairzx81 added the future Candidate future functionality label Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future Candidate future functionality
Projects
None yet
2 participants