creditkarma / thrift-typescript

Generate TypeScript from Thrift IDL files
Apache License 2.0
155 stars 32 forks source link

fix: update how type for temp object in encode is defined to fix strict unions #167

Closed hayes closed 5 years ago

hayes commented 5 years ago

@kevin-greene-ck this should fix #166

I could separate the logic for unions vs structs if you think it would be better to keep the previous logic for structs. I think the as cast is required for unions though.

hayes commented 5 years ago

well, this fixes one set of issues, but there is another issue now.

hayes commented 5 years ago

Now that there is proper type for the args object, this reveals another type issue with strict unions and default args. There is no valid way to pass an empty object to take advantage of the default. Working on a fix

hayes commented 5 years ago

@kevin-greene-ck This ended up being a little more complicated than I initially imagined.

There were 3 issues that came up when adding a type for the obj variable for encoding strict unions:

  1. The way to code is generated doesn't allow typescript to guarantee that only 1 property will be set.
  2. The Args interface for strict unions doesn't allow passing an empty object to use the default value for the union.
  3. The if condition around the check for handling assigning the default filters the type of obj. obj then can't be assigned the default value because it is now a more specific type that does not have those values set.

To fix this I did 3 things:

  1. I moved the type of obj into an as
  2. For unions with defaults I added a new IUnionNameDefaultArgs interface that has all values as undefined and included it in the args union type.
  3. I updated the default assignment for unions so that it assigns a new object to obj rather than setting a property. (eg https://github.com/creditkarma/thrift-typescript/pull/167/files#diff-8b97c3b8f61620cfd5530231d88df535R17)
hayes commented 5 years ago

@kevin-greene-ck any chance you could cut another pre-release with this change?

hayes commented 5 years ago

Bump

hayes commented 5 years ago

@kevin-greene-ck thanks for merging this, would you be able to cut a release with this change as well?