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

Inconsistency between RSVP.Promise and native promises #17021

Closed
boris-petrov opened this issue Oct 1, 2018 · 4 comments
Closed

Inconsistency between RSVP.Promise and native promises #17021

boris-petrov opened this issue Oct 1, 2018 · 4 comments

Comments

@boris-petrov
Copy link
Contributor

After reading the last posts in here:

https://discuss.emberjs.com/t/readers-questions-why-does-ember-still-use-rsvp/14736

I tried removing all mentions of RSVP.Promise in our code base. We are using the latest Ember.js (3.4.4) and the latest Ember-CLI (3.4.3). I have at least one case where there is still some difference between the two. I've put up a reproduction repo:

https://github.com/boris-petrov/ember-promise-bug

Running ember test causes one failure. If you uncomment import { Promise } from 'rsvp'; in the test, it passes. Which means that using a native promise is different than using an RSVP one. By reading the reader's question, I was under the impression that since Ember 3.4 they should be interchangeable.

Also, you can see that resolve('text!'); is commented out - if you uncomment it (and comment out the line above it which resolves the promise in a later) the test passes using both Promise types. Not sure why and how all this is connected...

Any insight would be appreciated!

@ef4
Copy link
Contributor

ef4 commented Oct 12, 2018

Thanks for taking the time to reproduce.

This code is racy. The test does not await the asynchronous behavior in the helper. I made a PR to your reproduction that shows how to make the test reliable.

I need to clarify what I said about native Promises: they absolutely work fine in Ember, and the helper in your example behaves correctly when implemented with a native Promise. But they don’t have identical timing, so if you are relying on races or side effects, you can observe different behavior. So the caveat is: you can switch from RSVP.Promise to Promise as long as you are waiting for the promise to resolve.

@ef4 ef4 closed this as completed Oct 12, 2018
@boris-petrov
Copy link
Contributor Author

boris-petrov commented Oct 12, 2018

@ef4 - thank you very much for the explanation! That definitely resolved our issue we had. I should have thought about the fact that the test doesn't wait for native promises but it does for RSVP ones.

A question while we're at it - I'm currently trying to rewrite a bunch of our CoffeeScript to TypeScript and I noticed that some classes/interfaces are defined as taking/extending RSVP.Promise instead of just Promise/PromiseLike - most notably PromiseProxyMixin. This breaks my TypeScript code and I have to suppress the warnings. So:

  1. It is fine using PromiseProxyMixins with native Promises I think?
  2. Are you going to change the type definitions now that RSVP is not "required" and as I read it will eventually be even removed as jQuery?

@ef4
Copy link
Contributor

ef4 commented Oct 13, 2018

I'm not sure what the plan is for PromiseProxyMixin. It should work fine with native promises, but its public API proxies a method (finally) that isn't on native Promise, so I don't know how to make typescript happy about that. Seems worth opening a separate issue to discuss.

@chriskrycho
Copy link
Contributor

I've added a comment over on the ember-cli-typescript issue linked above in this thread clarifying why our types are what they are and what some possible workarounds may be.

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

3 participants