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

Switch riot-web to bluebird #4565

Merged
merged 8 commits into from Jul 13, 2017

Conversation

@richvdh
Copy link
Member

richvdh commented Jul 12, 2017

As per matrix-org/matrix-js-sdk#490, we're switching to bluebird.

richvdh added 6 commits Jul 12, 2017
update `package.json`

```
find src test -name '*.js' |
    xargs perl -i -pe 'if (/require\(.[qQ].\)/) { $_ = "import Promise from '\''bluebird'\'';\n"; }'

find src test -name '*.js' |
    xargs perl -i -pe 'if (/import [qQ] /) { $_ = "import Promise from '\''bluebird'\'';\n"; }'
```
```
find src test -name '*.js' | xargs perl -i -pe 's/\b[qQ]\(/Promise.resolve(/'
```
```
find src test -name '*.js' |
    xargs perl -i -pe 's/q\.(all|defer|reject|delay|try|isFulfilled)\(/Promise.$1(/'
```
Bluebird doesn't have an `allSettled` method, so instead catch the exceptions and
use `all`.
turns out that you could call defer.resolve on q defers as an unbound function,
whereas that doesn't work with bluebird promises.
Copy link
Member

dbkr left a comment

otherwise lgtm

@@ -440,7 +442,7 @@ module.exports = React.createClass({
if (needsUpdate.length > 0) {
// If some of the rules need to be ported then wait for the porting
// to happen and then fetch the rules again.
return q.allSettled(needsUpdate).then( function() {
return q.all(needsUpdate).then( function() {

This comment has been minimized.

Copy link
@dbkr

dbkr Jul 13, 2017

Member

You missed a 'q'. Also surely allSettled != all? Looks like you want reflect (which used to be settle)

This comment has been minimized.

Copy link
@richvdh

richvdh Jul 13, 2017

Author Member

You missed a 'q'.

Oops.

Also surely allSettled != all? Looks like you want reflect (which used to be settle)

the reflect stuff looks confusing to me, so I tried to avoid it, and instead added the catch handler at 1d2d086#diff-beb48c1c7c8677b581a5b4177c817f28R434 to make the promises all succeed.

This comment has been minimized.

Copy link
@dbkr

dbkr Jul 13, 2017

Member

ah, I see

@dbkr dbkr assigned richvdh and unassigned dbkr Jul 13, 2017
@richvdh richvdh merged commit 348ec52 into develop Jul 13, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@richvdh richvdh deleted the rav/bluebird branch Jul 13, 2017
richvdh added a commit that referenced this pull request Jul 13, 2017
It turns out that the assertion made in
#4565 about `async` functions
returning bluebird promises was only correct when babel used an inline version
of the `asyncToGenerator` helper; in react-sdk we are using
`babel-transform-runtime` which means that we use a separate
`babel-runtime/helpers/asyncToGenerator`, which returns a native (or core-js)
Promise.

This meant that we were still in the situation where some methods returned
native Promises, and some bluebird ones, which is exactly the situation I
wanted to resolve by switching to bluebird in the first place: in short,
unless/until we get rid of all code which assumes Promises have a `done` method
etc, we need to make sure that everything returns a bluebird promise.

(Aside: there was debate over whether in the long term we should be trying to
wean ourselves off bluebird promises by assuming all promises are native. The
conclusion was that the complexity hit involved in doing so outweighed any
benefit of a potential future migration away from bluebird).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.