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

Conjure "set" generates a Typescript "Array" #98

Open
Ricky-N opened this issue Jul 10, 2019 · 2 comments
Open

Conjure "set" generates a Typescript "Array" #98

Ricky-N opened this issue Jul 10, 2019 · 2 comments

Comments

@Ricky-N
Copy link

Ricky-N commented Jul 10, 2019

What happened?

Generated typescript for conjure set specification is an Array, which can mislead developers to assume explicit ordering that is not guaranteed by the api contract.

This response object

ResponseObject:
    fields:
        myField: set<Thing>

Generates this typescript

export interface IResponseObject {
    'myField': Array<IThing>;
}

For developers who aren't referring back to the original conjure apis, but relying on the typescript definitions for code-safety, this creates opportunities for bugs that may not pop up until down-the-line when the backend service owners change internal ordering mechanics.

What did you want to happen?

The generated typescript should generate a Set that has the same semantic expectations as the original api contract specification.

@ferozco
Copy link
Contributor

ferozco commented Jul 10, 2019

Agreed that it would be nice if we did generate Sets in bindings but there are a couple reasons we can not.

  • The response is parsed as JSON and without Serialization/deserialization layer (translate the string "NaN" to the js Number.NaN value) #48 we would not be able to convert from a javascript array to a proper Set
  • ES Sets use reference equality for any complexity object, which can be unexpected and can result in multiple identical (i.e. deep equality) objects in a set
  • Changing the bindings (with the associated runtime changes) would be a breaking change to the bindings and internally breaks are incredibly painful to roll out

@Ricky-N
Copy link
Author

Ricky-N commented Jul 10, 2019

Ack on the challenges -- I wanted to make sure I filed this as part of P0 remediation. The problem popped up because a FE we maintain was relying on the implicit set ordering, which worked reasonably well until the backend added a filtering step that broke that order.

In looking to the root cause, I had a hard time fully blaming "bad dev practices" as the FE api explicitly specifies an Array type, and I could imagine someone who isn't going back to reference the conjure api might make the poor choice to assume some order, and trust that ordering.

This seems like a reasonable "wontfix", or at least a "delayed until a big break" kind of thing, but I think its worth tracking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants