feat: add support for maps #36
Conversation
|
I need to study this a bit more. On the one hand, there is going to be some complication with this change and i get that. On the other, this is a lot of code, and moving some of the rendering to the helpers adds some indirection. I also feel like there is a lot of repetitive code in here and there might be some simpler ways to do this. |
| if (property.$ref) { | ||
| tp = property.$ref.name | ||
| } else if (property.additionalProperties) { | ||
| tp = `Record<string, ${toTypeScriptType(property.additionalProperties)}>` |
There was a problem hiding this comment.
Why use Record over Map? my understanding is Record is pretty limited to the key and value types it can hold.
There was a problem hiding this comment.
I think i see the logic here. looks like keys can only really be strings anyway.
There was a problem hiding this comment.
I don't have a strong opinion here, but it seems like Record has a more straightforward path for json serialization: https://stackoverflow.com/questions/50153172/how-to-serialize-a-map-in-javascript
| if (!obj) return result | ||
|
|
||
| for (const [key, value] of Object.entries(obj)) { | ||
| result[key] = cast(value); |
There was a problem hiding this comment.
I'm not sure it's safe to mutate this and we may need to copy. we'll be changing the type of a property that a user my still be holding right?
There was a problem hiding this comment.
We are mutating the local copy result, so i think we should be fine
Yeah I agree, I went ahead and refactored the code a bit |
|
Putting this feature on ice for a little bit |
|
Closing in favor of #37 |
2 participants
Related to https://github.com/dylibso/xtp/pull/852