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

WrapResponses in TypeScript Client #600

Closed
ml-dev opened this issue Feb 16, 2017 · 22 comments
Closed

WrapResponses in TypeScript Client #600

ml-dev opened this issue Feb 16, 2017 · 22 comments

Comments

@ml-dev
Copy link

ml-dev commented Feb 16, 2017

I would like to use the

  • WrapResponses
  • GenerateResponseClasses
  • ResponseClass

settings when generating a TypeScript client. I see the settings are only available to the CSharp client generation.

I need to read the response headers for paging information and status codes.

@RicoSuter RicoSuter added this to the vNext milestone Feb 18, 2017
@BringerOD
Copy link

Same here. +1

@ml-dev
Copy link
Author

ml-dev commented Apr 20, 2017

@RSuter Any timeframe on getting this feature worked into the TypeScript generator?

@BringerOD
Copy link

This is pretty import with restful services.

@quails4Eva
Copy link
Contributor

I have implemented this for the Angular client in my branch, though I have only really tested it on the project I am working on and haven't tried all the different combinations of options. Are there some more examples I should test it on? Should I make a pull request?

quails4Eva@d3eddb7

@RicoSuter
Copy link
Owner

RicoSuter commented Aug 14, 2017

Looks good so far, please remove/do not commit the changes in the .tt autogenerated .cs files to make it more readable.. we should at least also support this for the fetch template. Thank you very much.

@RicoSuter
Copy link
Owner

Please create a PR and ill look into it.

@RicoSuter
Copy link
Owner

Ref: #891

@RicoSuter RicoSuter changed the title WrapSuccessResponses in TypeScript Client WrapResponses in TypeScript Client Aug 22, 2017
@quails4Eva
Copy link
Contributor

Thanks, done.

I did start looking into what's needed for Fetch, but only got about as far as realising that it isn't the same thing as Aurelia (I've been fairly busy recently).

@RicoSuter
Copy link
Owner

Ok, the feature is implemented in the master branch. Now we have to test this. Guys? :-)

@RicoSuter
Copy link
Owner

Please open specific issues if you encounter problems with this feature...

@RicoSuter
Copy link
Owner

Should be supported for all TypeScript templates

@RicoSuter
Copy link
Owner

And also added WrapResponseMethods setting

@quails4Eva
Copy link
Contributor

I tested the new version and it seems to work fine for what I want.
I notice that the keys are all coming through as lower case regardless of the casing in the headers themselves, I guess this is probably deliberate?
I also notice that the header is now a plain object rather than an Angular Header object, presumeably for consistency with the other clients.
I don't have any problem with either of those, they were just the two things I had to update when using the new code.

@ml-dev
Copy link
Author

ml-dev commented Aug 30, 2017

This looks great! Thanks @quails4Eva and @RSuter for getting these change implemented!

@RicoSuter
Copy link
Owner

  1. Using a plain headers object makes it easier to use it in all templates and id like to keep the api of the generated clients as technology independent as possible
  2. The casing of the headers is not changed - it seems that angular converts them (?)

@RicoSuter
Copy link
Owner

Btw: You can use WrapResponseMethods to enable this feature for specific operations

@quails4Eva
Copy link
Contributor

  1. I Googled it and you are correct. I think the Angualr header class was probably ignoring the case of the keys I passed which was why my mixed case keys were working before.

@RicoSuter
Copy link
Owner

So the generated code is fine?

@quails4Eva
Copy link
Contributor

Yes, it works fine for me.

@RicoSuter
Copy link
Owner

Do you think the casing is the same between different browsers? Otherwise we have to lower case all keys for safety...

@quails4Eva
Copy link
Contributor

quails4Eva commented Aug 31, 2017

I have done some testing (and reading up).
Apparently Chrome always converts headers to lower case; Firefox, IE and Edge were not doing this in my testing. But headers are supposed to be case-insensitive so Firefox and IE are handling this some other way.

So to get it to work consistently across browsers you need to have some Header class that abstracts away the casing, or you can probably just lower case them all. Angular does the former, but if Chrome is lowercasing them anyway then you can never guarentee you are getting the original casing if you support Chrome, so lower casing should be fine.

@RicoSuter
Copy link
Owner

OK, I've created a new issue: #916

We should implement this fast to avoid breaking changes for users...

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

No branches or pull requests

4 participants