Skip to content
This repository has been archived by the owner on Jun 10, 2022. It is now read-only.

[minor] Make PrimitiveDefinition a discriminated union #463

Merged
merged 1 commit into from
Oct 31, 2019

Conversation

eanders-ms
Copy link
Contributor

It was bothering me that invalid fields like uSegments were showing up in intellisense when creating a box primitive, so I modified the type definition to discriminate by primitive type.

At the same time, I changed ColliderType to be an enum, to align with our newly established coding guideline that we don't use string-union types where enums would suffice.

TODO: ColliderType needs docstrings.
TODO: *PrimitiveDefinition needs better docstrings.

Copy link
Contributor

@stevenvergenz stevenvergenz left a comment

Choose a reason for hiding this comment

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

As long as old code still compiles, I like this.

}
};

export type PrimitiveDefinition
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a very friendly data structure, which is why the assets API doesn't use it. I would be in favor of leaving this unchanged as a network type, then having a separate discriminated union for the CreatePrimitive API. That way we could have a radius field for example, but still have a unified data structure for the network message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that approach. I was wondering why we didn't have a radius field.

@eanders-ms
Copy link
Contributor Author

@stevenvergenz old apps that use primitives will still compile, but old apps that use colliders won't due to the change to ColliderType from union to enum, but I think it's a good change and worth it.

This change was just meant to clean up our API a little bit (ColliderType change) and improve intellisense (PrimitiveDefinition change). I like your suggestion above about redefining the PrimitiveDefinition types to be more friendly, but that's beyond scope for this.

No network payloads were changed, so it's backward compatible from that perspective.

@eanders-ms eanders-ms merged commit f16fcf5 into red Oct 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants