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

Intent to RFC: Deprecate RSVP #539

Closed
pzuraq opened this issue Sep 5, 2019 · 17 comments
Closed

Intent to RFC: Deprecate RSVP #539

pzuraq opened this issue Sep 5, 2019 · 17 comments
Labels
Seeking Co-author T-deprecation T-framework RFCs that impact the ember.js library

Comments

@pzuraq
Copy link
Contributor

pzuraq commented Sep 5, 2019

  • Proposed Target: Ember v4.0.0
  • Alternative: Native Promises

RSVP has been Ember's polyfill for promises since v1.0.0, before native promises existed in any browser, much less all of the supported ones. Native promises are now well supported in every browser we support, and much plumbing has been done to make sure that Ember interoperates nicely with them.

Given RSVP is a fairly decent size chunk of code (~3.5kb min+brotli), and that other polyfill solutions such as core.js exist now which provide much more seamless polyfills, it makes sense to do a final push to remove it in our next major version.

Migration Path

For the most part, native promises have a 1-1 API with RSVP. There are a couple of exceptions, such as RSVP.hash, but these are very minimal. As part of this RFC, we could consider extracting them into a plain-JS promise helpers library, if one doesn't exist already.

@NullVoxPopuli
Copy link
Sponsor Contributor

Nice. I actually just wrote a codemod for this :)

@buschtoens
Copy link
Contributor

One potential replacement for RSVP.hash is p-props. Sindre's got a huge collection of promise-related utilities here: promise-fun

For the "officially" recommended replacements, we should consider bundle file size. For instance, p-props adds 3 kb (1.3 kb gzip).

@pzuraq
Copy link
Contributor Author

pzuraq commented Sep 5, 2019

Can either of these libraries be treeshaken? With Embroider around the corner, I especially think utility function lib size will become less and less of an issue in the next year

@lolmaus
Copy link

lolmaus commented Sep 9, 2019

One important feature of RSVP is being able to give meaningful labels to promises, so that they can be debugged easily in Ember Inspector.

Uhm, I no longer see promise names in Ember Inspector. Has that been removed already? ☹️

@lolmaus
Copy link

lolmaus commented Sep 9, 2019

And will promise tracing still be available in the first place?

Not sure how Ember Inspector does that, I assume it taps into RSVP instrumentation.

@pzuraq
Copy link
Contributor Author

pzuraq commented Sep 9, 2019

This would likely mean removing custom instrumentation in the Inspector for standard browser instrumentation. Keep in mind, when the Inspector first implemented this, the browser provided nothing for promises. Today, when you look at a callback for a promise, you see a stack trace in the call stack which actually is capable of linking you back to where that promise was created.

The Inspector's experience is also already broken in any application that creates a native promise which doesn't get wrapped using RSVP, e.g. if you ever do new Promise() without importing it directly from RSVP, or if you ever use an async function, or if you use a library that isn't Ember specific 😕 unfortunately, I don't think it's possible for us to continue maintaining or even providing this aspect of the Inspector in any consistent way.

@runspired
Copy link
Contributor

Many of the commonly used rsvp utilities do not yet have native equivalents, although most of these utils them have standards work still progressing. I'd prefer to see us shift RSVP to being a utility lib that hopefully becomes droppable as standards improve.

I'd also note a few large drawbacks to dropping widespread use of RSVP promises. I don't think these are blockers but they are something which Ember's scheduling mechanisms will need to account for.

1 - lack of a coordinated flush increases risk of multiple-renders
2 - promise chain cancelling
3 - native promises do not expose promise state flags
4 - less/no insight into rejected promise chains (native has no equivalent to what the inspector provides)
5 - lack of promise labeling (a very nice thing in debug)

1 I investigated various mechanisms of "awaiting settledness" previously, we would want to take those investigations further.
2 is probably achievable with a custom interceptor to break the chain, would have to think on it. We have great need of this in ember-data. I think I previously found a way to refactor away from our use of private RSVP APIs for the future, will revisit.
3 we could expose a little wrapper for migrating folks
4/5 are something we could probably solve with a nice set of debug utils to reduce overhead but still provide a nice inspector experience.

@stefanpenner
Copy link
Member

@runspired ya good points. I personally would love to kill RSVP, but the points above, especially 1. is the most concerning.

@pzuraq
Copy link
Contributor Author

pzuraq commented Sep 10, 2019

Re: debug utils, I really think that unless we can somehow provide the experience of tracking promises like in the inspector for all promises in the browser, it’s not a good idea, as it has the potential to mislead users. What happens when a promise is failing or causing issues, and a user looks in the inspector and sees nothing? If we could wrap promises somehow in debug builds or via the inspector, I think that would make more sense, I’d support that more so.

Re: the scheduling issues, it was my understanding that the recent work in backburner had made them less problematic, maybe @krisselden/@rwjblue can shed more light there. I definitely don’t think we can remove the ability to coordinate flushes if it’ll result in worse performance though. Would it be possible to make a more minimal wrapper around native promises that preserves this ability, if needed? (Ideally it’s not needed)

@chriskrycho
Copy link
Contributor

3 is definitely worth considering in terms of shipping recommended utilities for migration—possibly also for general use. (Third party alternatives also no doubt could work here.) I've several times now needed to build utilities to handle it for components which want to do async-await style presentation of code, especially where Ember Concurrency tasks aren't available. Doing that in a way that works with native Promises is very possible… and also very annoying, precisely because of that. If we could extract that into some functional utilities (that could perhaps be used as helpers?) that'd be a super useful tool regardless.

@pzuraq pzuraq added T-deprecation T-framework RFCs that impact the ember.js library Seeking Co-author labels Oct 8, 2019
@pzuraq pzuraq changed the title Pre-RFC: Deprecate RSVP Intent to RFC: Deprecate RSVP Oct 8, 2019
@locks locks added this to Deprecations in Deprecation Candidates Oct 9, 2019
@mehulkar mehulkar mentioned this issue Apr 1, 2020
@locks locks moved this from Deprecations to Reviewed in Deprecation Candidates Dec 7, 2020
@mehulkar
Copy link
Contributor

IE11 is no longer supported, so I suppose we could remove RSVP now if it's still there?

@ldong
Copy link

ldong commented Sep 23, 2021

Removing polyfill is one thing, breaking existing APIs is another. Please be mindful when deprecating "RSVP", thanks.

@jherdman
Copy link

We have more than a few usages of RSVP.hash in our code for which there is currently no native replacement.

@NullVoxPopuli
Copy link
Sponsor Contributor

I could see migration path just being adding rsvp to your package.json

@snewcomer
Copy link
Contributor

Note - we have introduced an RFC for ember-data.

#796

I'd be happy to champion this issue on the ember side.

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

@snewcomer would you be up for writing the RFC for this?

@wagenet
Copy link
Member

wagenet commented Jul 29, 2022

I believe this falls under the umbrella of #832. I'm going to close this issue to push any discussion there. Despite being closed, we will certainly use this issue for reference and to inform the future RFC. Thank you all for the discussion!

@wagenet wagenet closed this as completed Jul 29, 2022
Deprecation Candidates automation moved this from Reviewed to Finished Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Seeking Co-author T-deprecation T-framework RFCs that impact the ember.js library
Projects
No open projects
Development

No branches or pull requests