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

[SEMVER-MAJOR] Always use bluebird as promise library #1896

Merged
merged 1 commit into from
Jan 8, 2016

Conversation

jannyHou
Copy link
Contributor

This pr is created for updating files to use bluebird as promise library.
See details here:
Connect to strongloop-internal/scrum-loopback#615

Related: loopbackio/loopback-datasource-juggler#790

@superkhau
Copy link
Contributor

LGTM but @bajtos should confirm this one before you merge. Also, why so many fails on CI?

@bajtos bajtos self-assigned this Dec 21, 2015
@bajtos bajtos changed the title Always use bluebird as promise library [SEMVER-MAJOR] Always use bluebird as promise library Dec 22, 2015
@@ -48,6 +48,3 @@ assert.isFunc = function(obj, name) {
assert(typeof obj[name] === 'function', name + ' is not a function');
};

if (!('Promise' in global)) {
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the trailing empty line at L50.

@bajtos
Copy link
Member

bajtos commented Dec 22, 2015

@jannyHou code changes LGTM. Please rebase your patch on top of the current master and add an entry to 3.0-RELEASE-NOTES.md. The idea is write a short text explaining loopback users (developers) what this change means for their applications and provide instructions how to upgrade. Also include a link to this pull request to make it easy to look up more details if needed in the future.

@superkhau could you please help here?

@@ -34,6 +34,7 @@
"dependencies": {
"async": "^0.9.0",
"bcryptjs": "^2.1.0",
"bluebird": "^2.9.9",
Copy link
Member

Choose a reason for hiding this comment

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

Please use 3.x version - the same as we do in juggler (loopbackio/loopback-datasource-juggler#790)

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't notice the older version of bluebird here. As suggested by @bajtos, please use 3.1.1 (latest on NPM - I checked for you).

@bajtos
Copy link
Member

bajtos commented Dec 22, 2015

code changes LGTM

"mostly good" would be a better description - we should upgrade to bluebird@3.x as part of this patch, see the comment above

@bajtos bajtos assigned superkhau and unassigned bajtos Dec 22, 2015
}

var promise = new global.Promise(function(resolve, reject) {
var promise = new Promise(function(resolve, reject) {
cb = function(err, data) {
if (err) return reject(err);
return resolve(data);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the return is necessary here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine to leave this since it's in the original code and not part of your changes anyways.

@superkhau
Copy link
Contributor

@superkhau could you please help here?

Sure. @jannyHou Ping me when you're working on this.

@superkhau superkhau assigned jannyHou and unassigned superkhau Jan 5, 2016
@jannyHou jannyHou force-pushed the feature/upgrade-to-bluebird branch 2 times, most recently from 8b48981 to 680317f Compare January 8, 2016 15:56
Replace `global.Promise` with `bluebird`
@jannyHou jannyHou force-pushed the feature/upgrade-to-bluebird branch from 680317f to 889c561 Compare January 8, 2016 19:13
jannyHou pushed a commit that referenced this pull request Jan 8, 2016
[SEMVER-MAJOR] Always use bluebird as promise library
@jannyHou jannyHou merged commit 70984bd into master Jan 8, 2016
@jannyHou jannyHou removed the #review label Jan 8, 2016
@jannyHou jannyHou deleted the feature/upgrade-to-bluebird branch May 2, 2016 20:26
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.

3 participants