Add Span.toObject function, replacing Span.toJSON #592
Conversation
The `Span.toJSON` function required us to always parse it manually. Why not do that in the function itself? That's what `Span.toObject` does. Other name suggestions are welcome! This way we don't cheat in tests and try and match on the JSON string with `toMatch(/substring/)`. Now we need to specify which attribute value we want to match. All fields of the SpanData object are optional because if no span data is returned by the extension, all fields are empty.
| * The internal data structure of a `Span` inside the AppSignal Extension. | ||
| */ | ||
| export type SpanData = { | ||
| closed?: boolean |
There was a problem hiding this comment.
The closed attribute should not be optional, I think, as it's present even when the span has been garbage-collected (or at least that's what toObject says)
Side note: one could also do something like
type SpanData = ClosedSpanData | OpenSpanData
type ClosedSpanData = { closed: false }
type OpenSpanData = {
closed: true,
name: string,
...
}This way, once the tests check that closed != false (for example, by throwing an error if it's closed) then the compiler will infer it's an OpenSpanData from that point onwards, and the other attributes can be accessed without the .! syntax.
I've been playing with this idea in the TypeScript playground for a bit. None of this is necessary, I just think discriminated unions are neat.
There was a problem hiding this comment.
The closed attribute should not be optional, I think, as it's present even when the span has been garbage-collected (or at least that's what toObject says)
It's not set if no data is returned from the extension at all. Would fit that into the union?
There was a problem hiding this comment.
Huh, adding the empty object as an option makes this surprisingly tricky. I've tried fitting it into the union in several ways, by adding an additional EmptyThing = {}, or by making the closed property optional in the ClosedThing object, or some mix of the two. Thing is, it seems like if it's optional, it can't be a discriminant the compiler will still consider it possible, even if you check for undefined. And if there's an EmptyThing declaration in the union, then you can't easily check for the property -- it's a type error to try to access a property that doesn't exist in the object.
The only way I've been able to make it work in a way that is type-sound is using this magically typed version of Object.hasOwnProperty that I found on the internet. It's a bit ugly, as you have to re-implement hasOwnProperty just to make the compiler happy. Here's a playground showcasing it.
That said, you can use type guards, like isOpen in these examples, to define your own rules for what makes one type a different type. This is simpler and also works.
There was a problem hiding this comment.
Wow cool! But that seems like a lot for something we only use in the test suite, right?
There was a problem hiding this comment.
Ah yes, definitely! 😅 Sorry, got carried away. Type systems are fun, in their own way. Disregard the above.
The
Span.toJSONfunction required us to always parse it manually. Whynot do that in the function itself? That's what
Span.toObjectdoes.Other name suggestions are welcome!
This way we don't cheat in tests and try and match on the JSON string
with
toMatch(/substring/). Now we need to specify which attributevalue we want to match.
All fields of the SpanData object are optional because if no span data
is returned by the extension, all fields are empty.
Based on #591