Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

[typescript][jQuery] Fix promise resolution #581

Merged
merged 2 commits into from
Jul 19, 2018
Merged

[typescript][jQuery] Fix promise resolution #581

merged 2 commits into from
Jul 19, 2018

Conversation

grollinger
Copy link
Contributor

@grollinger grollinger commented Jul 17, 2018 •
edited
Loading

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.1.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

(Port of swagger-api/swagger-codegen#8385)

Resolves #581

@grollinger grollinger changed the title Fix promise resolution [typescript][jQuery] Fix promise resolution Jul 17, 2018
@macjohnny
Copy link
Member

cc @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01)

macjohnny
macjohnny approved these changes Jul 17, 2018
View reviewed changes
Copy link
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

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

looks good to me. this is a breaking change, but the return value did not correspond to the type, so this should be fine.

@macjohnny
Copy link
Member

@grollinger thanks for your PR. you could also add the typescript-jquery to https://github.com/ackintosh/openapi-generator-1/blob/master/bin/utils/ensure-up-to-date#L29 to have the CI check for each PR that the jquery samples have been generated correctly.

@macjohnny
Copy link
Member

the shippable build fails because samples/server/petstore/php-slim/phpunit.xml.dist is not up to date. @wing328 could you update the file? then the PR should be ready to be merged.

@grollinger
typescript-jquery: fix promise resolution
81e23ce 
The type annotation that is generated for an API states that the
JQueryPromise<...> will be resolved with a single value.
The implementation, on the other hand resolves with two.

This changes the implementation to resolve the promise in accordance
with the type annotation.
It also adds another type specifying what will be passed if the promise
is rejected.
@grollinger
Update petstore sample for typescript-jquery
3aeebb9
@grollinger grollinger force-pushed the fix-promise-resolution branch from 120a1c1 to 3aeebb9 Compare July 17, 2018 12:44
@wing328
Copy link
Member

wing328 commented Jul 17, 2018

the shippable build fails because samples/server/petstore/php-slim/phpunit.xml.dist is not up to date. @wing328 could you update the file? then the PR should be ready to be merged.

Thanks for pointing it out. We will take care of that

@wing328 wing328 added this to the 3.1.2 milestone Jul 19, 2018
@wing328 wing328 merged commit f7617bc into OpenAPITools:master Jul 19, 2018
@wing328
Copy link
Member

wing328 commented Jul 19, 2018

@grollinger thanks for the PR, which has been merged into master.

@macjohnny thanks for reviewing the PR.

@macjohnny macjohnny mentioned this pull request Jul 23, 2018
Closed
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
@grollinger @wing328
[typescript][jQuery] Fix promise resolution (OpenAPITools#581)
c92616c 
* typescript-jquery: fix promise resolution

The type annotation that is generated for an API states that the
JQueryPromise<...> will be resolved with a single value.
The implementation, on the other hand resolves with two.

This changes the implementation to resolve the promise in accordance
with the type annotation.
It also adds another type specifying what will be passed if the promise
is rejected.

* Update petstore sample for typescript-jquery
@snyk-bot snyk-bot mentioned this pull request Sep 21, 2021
Open
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@macjohnny macjohnny macjohnny approved these changes

Assignees
No one assigned
Labels
Client: TypeScript Issue: Bug
Projects
None yet
Milestone
3.1.2
Development

Successfully merging this pull request may close these issues.
3 participants
@grollinger @macjohnny @wing328