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

Make example code import and use the right modules in the right way #555

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ohcibi
Copy link

@ohcibi ohcibi commented Mar 18, 2020

The former example where not technically wrong but pretty misleading,
since they suggested Promise (the one that can be used in
new Promise) is the default export of the rsvp package when it isn't.
Ideally when using the default import it should be called RSVP rather
then Promise but in fact neither of the code example require to use
that default export at all so I removed using that in the examples
entirely.

The former example where not technically wrong but pretty misleading,
since they suggested `Promise` (the one that can be used in
`new Promise`) is the default export of the `rsvp` package when it isn't.
Ideally when using the default import it should be called `RSVP` rather
then `Promise` but in fact neither of the code example require to use
that default export at all so I removed using that in the examples
entirely.
@rwjblue rwjblue requested a review from stefanpenner March 29, 2020 15:59
@stefanpenner
Copy link
Collaborator

If the imports aren't working, that sounds like a bug. That should be fixed.

@ohcibi
Copy link
Author

ohcibi commented Mar 30, 2020

@stefanpenner this is not a bug in rsvp but an error in the documentation.

This line: https://github.com/tildeio/rsvp.js/blob/master/lib/rsvp.js#L47
tells that the default export of rsvp is only for legacy compat, i.e. there will be no default export anymore eventually (the old RSVP which this default export resembles wasn't an alias for Promise either, wasn't it?).

@stefanpenner
Copy link
Collaborator

I see, the goal is for the default export to be Promise but due to compat it is a complex object with many properties..

is fulfilled with an array of fulfillment values for the passed promises, or
rejected with the reason of the first passed promise to be rejected. It casts all
elements of the passed iterable to promises as it runs this algorithm.

Example:

```javascript
import Promise, { resolve } from 'rsvp';
import { all, resolve } from 'rsvp';
Copy link
Collaborator

@stefanpenner stefanpenner Apr 1, 2020

Choose a reason for hiding this comment

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

In our docs we should ensure import { X } from 'rsvp'; should be either Promise or utility methods unique to RSVP. The import { resolve, all , reject, race } from 'rsvp' are odd, and their is no reason for RSVP to encourage non-standard promise usage patterns.

Let's use all from Promise, as that will make it easier for people to move to native.

import { Promise } from 'rsvp';

Promise.all

Copy link
Author

Choose a reason for hiding this comment

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

@stefanpenner your example and my changes are only about all but in fact your logic applies for reject, resolve, race and such as well, doesn't it? My idea was to make the imports consistent by importing all the same way as the others were already imported in those examples but I see that it would make even more sense the other way around. In fact I haven't even seen something that RSVP exports which is unique to it but I will look into it once more. I guess in any case I should update the docs to be consistent for all native Promise functions, right?

@@ -28,14 +28,14 @@ import Enumerator from '../enumerator';
Example:

```javascript
import Promise, { resolve, reject } from 'rsvp';
import { all, resolve, reject } from 'rsvp';
Copy link
Collaborator

Choose a reason for hiding this comment

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

^

first passed promise to settle.

Example:

```javascript
import Promise from 'rsvp';
import { Promise, race } from 'rsvp';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's instead document Promise.race to better spec parity

settled promise matters. For example, even if other promises given to the
`promises` array argument are resolved, but the first settled promise has
become rejected before the other promises became fulfilled, the returned
promise will become rejected:

```javascript
import Promise from 'rsvp';
import { Promise, race } from 'rsvp';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's instead document Promise.race to better spec parity

@@ -65,9 +65,9 @@ import {
An example real-world use case is implementing timeouts:

```javascript
import Promise from 'rsvp';
import { Promise, race } from 'rsvp';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's instead document Promise.race to better spec parity

@@ -24,9 +24,9 @@ import {
Instead of writing the above, your code now simply becomes the following:

```javascript
import Promise from 'rsvp';
import { reject } from 'rsvp';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's instead document Promise.reject to better spec parity

@@ -22,9 +22,9 @@ import {
Instead of writing the above, your code now simply becomes the following:

```javascript
import Promise from 'rsvp';
import { resolve } from 'rsvp';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's instead document Promise.resolve to better spec parity

@locks locks self-assigned this Apr 10, 2020
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

Successfully merging this pull request may close these issues.

4 participants