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

RFC: ember data deprecate RSVP.Promise for native Promises #796

Merged
merged 1 commit into from
Mar 31, 2022

Conversation

snewcomer
Copy link
Contributor

@snewcomer snewcomer commented Feb 20, 2022

@snewcomer snewcomer added the T-ember-data RFCs that impact the ember-data library label Feb 20, 2022
@snewcomer snewcomer self-assigned this Feb 20, 2022
@snewcomer snewcomer force-pushed the sn/deprecate-rsvp branch 3 times, most recently from 02a2ae6 to e5313dd Compare February 20, 2022 16:19

By removing `RSVP.Promise` in favor of native Promises, we can drop an unnecessary dependency for both client side and server side fetching of data.

According to [bundlephobia](https://bundlephobia.com/package/[email protected]), this would allow us to remove a significant chunk of dependency weight.
Copy link
Contributor

Choose a reason for hiding this comment

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

text/0796-ember-data-deprecate-rsvp.md Show resolved Hide resolved
text/0796-ember-data-deprecate-rsvp.md Outdated Show resolved Hide resolved
Copy link
Contributor

@locks locks left a comment

Choose a reason for hiding this comment

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

Some minor tweaks!

text/0796-ember-data-deprecate-rsvp.md Outdated Show resolved Hide resolved
text/0796-ember-data-deprecate-rsvp.md Outdated Show resolved Hide resolved
text/0796-ember-data-deprecate-rsvp.md Show resolved Hide resolved

## How we teach this

We do not believe this requires any update to the Ember curriculum. API documentation may be needed to remove traces of `RSVP.Promise`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to include the information that goes into the deprecation guide.
You will also need to document the optional feature, especially if you want to flip the availability down the line, right?


## How we teach this

We do not believe this requires any update to the Ember curriculum. API documentation may be needed to remove traces of `RSVP.Promise`.
Copy link

Choose a reason for hiding this comment

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

It has been my experience that many feel that RSVP offers two features not available in native Promises: .hash() and test waiters support (await settled() support).

If this is not that case—

  1. Can we add to the "How we teach this" how to support .hash() or offer an alternative using native promises.
  2. Can we add ti the "How do we teach this" an explanation why native promises are test-waiter compatible and that the integration that RSVP offered is no longer needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sukima

  1. whether we return a native promise or an rsvp promise does not change anyone's ability to use hash.
  2. good call out, it's unnecessary because the test waiters aren't waiting on RSVP Promises in this case either, they are waiting on various other things, which ember-data already installs waiters for.

Copy link

Choose a reason for hiding this comment

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

Oh! This RFC is for Ember-data I thought it was for Ember proper gotcha. 👍

@snewcomer snewcomer merged commit 77312dd into emberjs:master Mar 31, 2022
@snewcomer snewcomer deleted the sn/deprecate-rsvp branch March 31, 2022 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-ember-data RFCs that impact the ember-data library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants