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

removed returning await pattern from node templates #1036

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

sahilsekr42
Copy link

What does this PR do?

Solves issue #1035

Test Plan

Related PRs and Issues

#1035

Have you read the Contributing Guidelines on issues?

yes

Copy link
Member

@ChiragAgg5k ChiragAgg5k left a comment

Choose a reason for hiding this comment

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

@sahilsekr42 can we do the same for:

  • react-native:
    {% if method.type != 'location' and method.type != 'webAuth'%}async {% endif %}{{ method.name | caseCamel }}{{ method.responseModel | getGenerics(spec) | raw }}({% for parameter in method.parameters.all %}{{ parameter.name | caseCamel | escapeKeyword }}{% if not parameter.required or parameter.nullable %}?{% endif %}: {{ parameter | getPropertyType(method) | raw }}{% if not loop.last %}, {% endif %}{% endfor %}{% if 'multipart/form-data' in method.consumes %}, onProgress = (progress: UploadProgress) => {}{% endif %}): {{ method | getReturn(spec) | raw }} {
  • web:
    {% if method.type != 'location' %}async {% endif %}{{ method.name | caseCamel }}{{ method.responseModel | getGenerics(spec) | raw }}({% for parameter in method.parameters.all %}{{ parameter.name | caseCamel | escapeKeyword }}{% if not parameter.required or parameter.nullable %}?{% endif %}: {{ parameter | getPropertyType(method) | raw }}{% if not loop.last %}, {% endif %}{% endfor %}{% if 'multipart/form-data' in method.consumes %}, onProgress = (progress: UploadProgress) => {}{% endif %}): {{ method | getReturn(spec) | raw }} {

@sahilsekr42
Copy link
Author

Hello , @ChiragAgg5k I guess we should be able to do it. However I thought of only making changes in the node sdk generator maybe we can raise other PR for that once this is merged. Thanks for review.

@ChiragAgg5k
Copy link
Member

@sahilsekr42 i can see why you might feel thats a good idea, but here its a bit different. the sdk generator repo is responsible for generating all the sdks at once, so whenever we do release a new version for it we release one for all the repos. so each change made if its similar across the repos, we make sure to change it across all of the them for consistency,

here is an example: #1031

its not a big change so do try to make the changes and i can help if you face any issues!

@sahilsekr42
Copy link
Author

Hey @ChiragAgg5k, I've made changes for react-native and web generator . Although there were some instances in react-native template where I felt 'await' might be required hence I chose to not change those parts . Please review and tell..

@ChiragAgg5k
Copy link
Member

@sahilsekr42 tests seem to be failing for web sdk, can you see why?

@sahilsekr42
Copy link
Author

Hello @ChiragAgg5k looks like all tests passed...

@ChiragAgg5k
Copy link
Member

@sahilsekr42 looks like you removed async notation from node sdk but not from web and react. does removing async result in better performance? if not can you add it back for node so its consistent?

@sahilsekr42
Copy link
Author

Actually it was necessary in react-native and web so I did not remove it..

@sahilsekr42
Copy link
Author

Thanks @ChiragAgg5k

@ChiragAgg5k
Copy link
Member

@sahilsekr42 for react-native i see we can additionally remove await call here -
https://github.com/sahilsekr42/sdk-generator/blob/a32062a59b39282e01ee8199330d403d8e3ddae7/templates/react-native/src/services/template.ts.twig#L96

for web i dont see any await calls. can you try removing async from there?

@sahilsekr42
Copy link
Author

For web, removing the async causes issues maybe because the way response is being handled by caller function (there were some httprequest issues for the ping() function) and for for RN, yes I'm doing it.

@sahilsekr42
Copy link
Author

Please see now @ChiragAgg5k

@sahilsekr42
Copy link
Author

Hey @ChiragAgg5k thanks for your time. Also some checks need approval by maintainers so please approve.

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

Successfully merging this pull request may close these issues.

2 participants