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 the Promise implementation #790

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.
Details see here:
Connect to strongloop-internal/scrum-loopback#615

Related: strongloop/loopback#1896

@bajtos bajtos self-assigned this Dec 15, 2015
@bajtos
Copy link
Member

bajtos commented Dec 15, 2015

Hi @jannyHou, I guess this is a good start. Now you need to clean up createPromiseCallback. We no longer depend global.Promise, thus there is no need to check if it's defined. Also please move bluebird from devDependencies to dependencies.

@bajtos
Copy link
Member

bajtos commented Dec 15, 2015

Also remove these three lines and make sure tests are still passing on Node v0.10: https://github.com/strongloop/loopback-datasource-juggler/blob/1e9bbd27873077ef374ea5487121a70173757364/test/init.js#L30-L32

@jannyHou
Copy link
Contributor Author

Hi @bajtos , I fixed 2 of 3 failures, and now tests pass on node v.10. Actually the third one is an unhandled error msg instead of a failure(at least mocha doesn't count it), and I get the same error when I run the code cloned from master branch. I guess it's not caused by my change but I'll see how to fix it(because it seems related to promise).

I commented if (!global.Promise) { //DoCheck } instead of delete, since I am not sure do we still need some check for promise library, I mean bluebird.

And I created a list of promisified apis whose doc might need update:
https://github.com/strongloop-internal/scrum-loopback/issues/615#issuecomment-165314984

Thanks!

"mocha": "^2.1.0",
"should": "^5.0.0"
},
"dependencies": {
"async": "~1.0.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.

While you are making this change, could you please upgrade Bluebird to 3.x?

@bajtos
Copy link
Member

bajtos commented Dec 17, 2015

I fixed 2 of 3 failures, and now tests pass on node v.10. Actually the third one is an unhandled error msg instead of a failure(at least mocha doesn't count it), and I get the same error when I run the code cloned from master branch. I guess it's not caused by my change but I'll see how to fix it(because it seems related to promise).

The new code looks right.

I commented if (!global.Promise) { //DoCheck } instead of delete, since I am not sure do we still need some check for promise library, I mean bluebird.

No need to keep that code in, we will always have the promise library as long as Node handles dependencies correctly (which is safe to assume).

And I created a list of promisified apis whose doc might need update:
strongloop-internal/scrum-loopback#615 (comment)

👍

@bajtos bajtos changed the title Use bluebird in utils.js [SEMVER-MAJOR] Always use bluebird as the Promise implementation Dec 17, 2015
@jannyHou jannyHou force-pushed the feature/upgrade-to-bluebird branch 2 times, most recently from bbee288 to deec6f4 Compare December 17, 2015 16:14
@@ -27,6 +27,3 @@ if (!('getModelBuilder' in global)) {
};
}

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 (L29) too.

@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 https://github.com/strongloop/loopback-datasource-juggler/blob/master/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.

@superkhau could you please help here?

@bajtos
Copy link
Member

bajtos commented Dec 22, 2015

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.


## Always use bluebird as promise library

In version 3.0, we always use bluebird as our promise library instead of `global.Promise`. We consider Bluebird API a part of LoopBack API from now on, you are welcome to use any Bluebird-specific methods in your applications.
Copy link
Member

Choose a reason for hiding this comment

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

Please keep line width at 80 characters max, it makes it easier to review diffs.

@bajtos
Copy link
Member

bajtos commented Jan 7, 2016

@jannyHou One more minor comment to address. Once you address it, please go ahead and land the PR without waiting for another review.

Replace `global.Promise` with `bluebird`
jannyHou pushed a commit that referenced this pull request Jan 8, 2016
[SEMVER-MAJOR] Always use bluebird as the Promise implementation
@jannyHou jannyHou merged commit 1771bfa into master Jan 8, 2016
@jannyHou jannyHou removed the #review label Jan 8, 2016
@superkhau superkhau mentioned this pull request Jan 20, 2016
@bajtos bajtos deleted the feature/upgrade-to-bluebird branch April 7, 2016 15:23
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