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.

Sign up for GitHub

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

Jump to bottom

0.8.0 exposes bluebird promises in API, breaking things #198

Closed
ddgenome opened this issue Jan 30, 2019 · 4 comments
Closed

0.8.0 exposes bluebird promises in API, breaking things #198

ddgenome opened this issue Jan 30, 2019 · 4 comments

Comments

@ddgenome
Copy link
Contributor

Commit f4c4a64 exposes bluebird promises as part of the externally facing API. This breaks downstream TypeScript projects in two ways:

  1. The @types/bluebird were not moved from "devDependencies" to "dependencies" in the package.json, see Typescript definitions are broken in 0.7.1 #156 for context.
  2. Bluebird promises are incompatible with ES6 Promises, see bluebird 3.0 definifion is not assignable to ES6 Promises DefinitelyTyped/DefinitelyTyped#11027, so downstream projects end up having to wrap, workaround, or use bluebird promises everywhere. The latter option is not viable if you use async/await since in those instances TypeScript requires that the return type be an ES6 Promise, see async / await and PromiseLike microsoft/TypeScript#5911.

The first issue is easy enough to fix. The second issue is a bit more complicated. Is there a compelling reason to use bluebird promises over native ES6 promises? If so, it is going to cause compilation failures in downstream projects.

I'd be happy to submit a PR to revert the addition of bluebird.
@itowlson
Copy link
Contributor

Sounds good to me. I maintain a downstream project which has had some hurt from this library exposing third-party types, and I could do without another round of the same...! @brendandburns this was your commit and it was a big regenerate - was it intentional to surface Bluebird, and if so is it okay to back it out? Is this going to need changes in the Swagger generator so it doesn't re-Bluebird us on the next regenerate?

ddgenome pushed a commit to atomist/sdm-pack-k8s that referenced this issue Jan 30, 2019
Update Kubernetes client, workaround bluebird
a3e088e 
@kubernetes/client-node@0.8.0 exposes bluebird promises in its API,
causing a lot of breaking changes.  See
kubernetes-client/javascript#198 for more
information.

This commit bites the bullet because exec-based auth is broken in
earlier versions.  The order of arguments for delete methods also
changed, so update that.
@brendandburns
Copy link
Contributor

Hrm, I'm not sure why the regenerate added bluebird promises to the API surface area.

@flyboarder do you have any ideas here?

@brendandburns
Copy link
Contributor

In looking at this, I think we can simply remove the import from api.ts and it will work. It won't be awesome for people who don't use es6 but I think they'll know what to do...

@brendandburns brendandburns mentioned this issue Jan 31, 2019
Merged
ddgenome pushed a commit to atomist/sdm-pack-k8s that referenced this issue Jan 31, 2019
Update Kubernetes client, workaround bluebird
d36355d 
@kubernetes/client-node@0.8.0 exposes bluebird promises in its API,
causing a lot of breaking changes.  See
kubernetes-client/javascript#198 for more
information.  Implement various fixes to workaround the issue.

This commit bites the bullet because exec-based auth is broken in
earlier versions.  The order of arguments for delete methods also
changed, so update that.
@burn2delete
Copy link
Contributor

@brendandburns no idea here, I just ran the generator so I imagine the problem is caused upstream by that. Where is bluebird introduced in the project and do we actually use it internally anywhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@ddgenome @burn2delete @itowlson @brendandburns