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

Use any promise #51

Closed
ligaz opened this issue Oct 6, 2016 · 20 comments
Closed

Use any promise #51

ligaz opened this issue Oct 6, 2016 · 20 comments

Comments

@ligaz
Copy link

ligaz commented Oct 6, 2016

Hey @spartan563,

Have you considered switching to any-promise instead of Bluebird?

@notheotherben
Copy link
Member

I haven't and am not sure that it would play particularly nicely with TypeScript (something which always needs to be taken into account with this project). Beyond that, Bluebird is the recommended promise implementation for this project right now simply as a result of its performance profile and I would be worried that allowing people to replace the promise implementation could lead to difficult to debug performance degradation in the wild. Is there a particular use-case you have for wanting to swap it out?

@ligaz
Copy link
Author

ligaz commented Oct 7, 2016

The main reason behind such a switch is that it will decouple the external interface of the library away from Bluebird it it (Bluebird) will be used only for in the internals.

For example in the project we are currently working on uses newer versions of Node.js that have promises built-in so there is no reason for us to include Bluebird as dependency, however we have to because the Iridium public API depends on it.

@notheotherben
Copy link
Member

Can certainly see where you're coming from, there are however reasons why one may wish to use a non-standard promise library (primarily performance and memory consumption related, but functionality is another big thing).

Irrespective of that though, I'll try and spend some time investigating whether there are tweaks I can make to ensure Bluebird in Iridium can be used in a standard Promises project. Can you confirm for me whether you're using TypeScript for this project or not?

@ligaz
Copy link
Author

ligaz commented Oct 7, 2016

Yes we are using TypeScript (2.0.3).

@notheotherben
Copy link
Member

Okay brilliant, and I assume the biggest issue you're facing isn't that Bluebird is used but rather that the promises returned by Iridium are typed as Bluebird promises, causing type cast errors?

@ligaz
Copy link
Author

ligaz commented Oct 7, 2016

@notheotherben
Copy link
Member

Okay, I just need to walk into work quickly and then I have a meeting, but as soon as that's done I'll take a look and get back to you when I have a solution.

@ligaz
Copy link
Author

ligaz commented Oct 7, 2016

Thanks. I see others are hitting similar issues #52.

If you need my help for anything please ping me.

@notheotherben
Copy link
Member

Hi Stefan,

I've just updated the Iridium examples repository to not directly use Bluebird and haven't seen any issues so far with the stock promise implementation. Could you maybe send me a short snippet of code showcasing what you're doing when you get the failure so that I can reproduce it?

You can view the examples here: https://github.com/SierraSoftworks/Iridium-Example/

In other words, I'm seeing that it is possible (at least in my case) to pass a Bluebird promise into a standard promise in TypeScript without issues.

@ligaz
Copy link
Author

ligaz commented Oct 7, 2016

You are not seeing any issues because the Promise global object is resolved from the Bluebird declaration file, so in fact you are returning a Bluebird promise.

@notheotherben
Copy link
Member

Right, so do you not have the @types/bluebird installed in your project?

@ligaz
Copy link
Author

ligaz commented Oct 7, 2016

If you install the latest version of @types/bluebird (3.0.35) in your sample project you will see the problem manifested. It looks like the global Promise variable was removed from the Bluebird declaration files.

@notheotherben
Copy link
Member

Brilliant, let me do that and see where I get 👍

@notheotherben
Copy link
Member

Looks like this is a known issue in the Bluebird typings - DefinitelyTyped/DefinitelyTyped#11027

Aside from a complete redesign of the external Iridium API (switching from Bluebird to native promises) which would require a version bump to 8.x (after we've just stabilized on the 7.x API), I think the only real solution is to fix this in Bluebird. In the mean time, I can put together a compatible Bluebird type definition which will allow you to work with it if you'd prefer?

@notheotherben
Copy link
Member

Another quick alternative I've found, but which is relatively hacky, is to add a .d.ts file to your project with the following in it.

declare module "bluebird" {
  export = Promise;
}

It doesn't appear to cause any issues with Iridium 7.0.0 in the example project, but that is by no means an extensive test.

@ligaz
Copy link
Author

ligaz commented Oct 10, 2016

Thanks, this workaround does the trick.

I believe the right long term solution is to move either to native Promises or adopt any-promise approach to decouple the public API from Bluebird.

@notheotherben
Copy link
Member

Possibly, more likely it's just a matter of re-typing the API to expose standardized promise types, this doesn't affect the functionality in any way and is purely a TypeScript artifact caused by the lack of the ToStringTag symbol on Bluebird's type definition.

@ligaz
Copy link
Author

ligaz commented Oct 10, 2016

That's right, but the actual Bluebird usage in the code is trivial (or at least this is what my quick search reveals 😄 ), so it might be a good idea to use the native Promise implementation and remove the Bluebird dependency in general.

@notheotherben
Copy link
Member

It may be simple, but it is used on a number of hot paths and Bluebird still knocks the reference Promise implementation out of the park in terms of both latency and memory usage, so there's a very specific reason it has been selected as the implementation of choice for Iridium's core.

Sources:

@ligaz
Copy link
Author

ligaz commented Oct 10, 2016

Fair enough. I completely agree that for something like an ORM performance plays a critical role.

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

No branches or pull requests

2 participants