search

creditkarma / thrift-typescript

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

[WIP] BREAKING CHANGE: refactor how annotations are exposed in generated TS #154

Closed kevin-greene-ck closed 5 years ago

kevin-greene-ck commented 5 years ago

This removes annotations from generated structs, unions and exceptions and adds a new property to services called metadata that contains information about the service including the service annotations and annotations associated with any methods or arguments to those methods.

It's best to look at the test fixtures to see how the changes present.

hayes commented 5 years ago

@kevin-greene-ck how set are you on removing annotations from structs?

We have been using thrift-typescript pretty heavily, but do not use the server or client. For our services, we use a lot of annotations on fields that get consumed by various libraries that assume they are accessible from the structs they consume. We do have our own codegen layer on top of thrift-typescript where we could add our own annotation processing, but having them directly on the structs is very useful.

I can add more detail about how we use annotations if that would be helpful.

hayes commented 5 years ago

If I am understanding this change correctly, this new approach loses the ability to access annotations on anything other than services and methods, is that correct?

eg the dot.lonely annotation in tests.

kevin-greene-ck commented 5 years ago

@hayes We're not set on it. I'll add them back. You'll notice this is going on 4.x branch. We have several changes in mind. I'm going to start a thread in issues this week with a list of what we have in mind now, open it for discussion and will keep it going until we're ready for the major bump, probably a couple months out.

kevin-greene-ck commented 5 years ago

The reason I was dropping the annotations from the structs here was we were thinking about making strictUnions the default in v4 and with that flag we don't render a class for unions. Was thinking about moving all that metadata over to the Codec/Toolkit object, but that may not be ideal if you actually need that data.

hayes commented 5 years ago

@kevin-greene-ck that makes sense. Thanks for explaining the motivation here. I think it does make sense to find a standardized way to access annotations that works for all types. I did some experimenting last night, and moving how we get annotations into our own compiler was pretty straight forward. Not having access to them will be pretty easy and clean to work around for our existing uses. I wonder if it would make sense to have the metadata live somewhere other than the client so that it can be made available for all types. Maybe a new object exported for the namespace?