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

Implement promise support #154

Merged
merged 15 commits into from Aug 22, 2016
Merged

Implement promise support #154

merged 15 commits into from Aug 22, 2016

Conversation

aydrian
Copy link
Contributor

@aydrian aydrian commented Aug 18, 2016

#107 Used bluebird.js. You can now use a callback or a promise. All the tests still pass but we might want to look at how and what we are testing.

@aydrian aydrian added this to the 2.0.0 Release milestone Aug 18, 2016
@coveralls
Copy link

coveralls commented Aug 18, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 35a602e on aydrian:issue-107 into 0564e25 on SparkPost:wip-2.0.0.

@jasonrhodes jasonrhodes self-assigned this Aug 18, 2016
@jasonrhodes
Copy link
Contributor

jasonrhodes commented Aug 19, 2016

Probably not in scope for this ticket, so we can file a separate refactor issue (happy to help with this), but look at what happens with the core lib fs in node, if you pass it a function as its first argument (it expects a string path first, then a function as a second arg):

> fs.readFile(function(err, buffer) { console.log(arguments); });
TypeError: path must be a string
    at TypeError (native)
    at Object.fs.readFile (fs.js:250:11)
    at repl:1:4
    at REPLServer.defaultEval (repl.js:262:27)
    at bound (domain.js:287:14)
    at REPLServer.runBound [as eval] (domain.js:300:12)
    at REPLServer.<anonymous> (repl.js:431:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:211:10)

It doesn't smartly hand that error to the function provided, because it wasn't provided in the right place. So I think we can ditch a lot of this kind of extra-cautious checking, a la:

if (typeof domain === 'function') {
  callback = domain;
  domain = null;
}

and simplify a lot of this, unless I'm missing a reason we need that.

@jasonrhodes
Copy link
Contributor

So this looks good, nice work @aydrian -- I've been looking through everything and went over the old discussions and did some testing locally. I have a file that's about 15-20 lines that can replace our bluebird dependency and keep all of your changes otherwise intact, with all tests passing. Are we ok with supporting nodejs 0.12.15+ with this new version (or 4.0+ even?)?

@aydrian
Copy link
Contributor Author

aydrian commented Aug 19, 2016

Let's create another ticket for the refactor if you can generalize that issue. I'm sure it's in many places. And there's a lot yet to refactor.

I am all for discontinuing support for versions 0.10 and 0.12. I have issue #153 for that.

What is the benefit to not using bluebird? All the research I've done shows that they are more performant than native promises. http://programmers.stackexchange.com/questions/278778/why-are-native-es6-promises-slower-and-more-memory-intensive-than-bluebird

Replaced bluebird with native promise support for PR SparkPost#154
@coveralls
Copy link

coveralls commented Aug 22, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling f54b79f on aydrian:issue-107 into 0564e25 on SparkPost:wip-2.0.0.

@coveralls
Copy link

coveralls commented Aug 22, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 778bcd2 on aydrian:issue-107 into 0564e25 on SparkPost:wip-2.0.0.

@aydrian
Copy link
Contributor Author

aydrian commented Aug 22, 2016

This breaks the build for v.10 and v.12, but we will be remove support in a different ticket.

@aydrian aydrian merged commit 23fdf26 into SparkPost:wip-2.0.0 Aug 22, 2016
@aydrian aydrian deleted the issue-107 branch August 22, 2016 15:06
@aydrian aydrian mentioned this pull request Aug 22, 2016
aydrian added a commit that referenced this pull request Oct 27, 2016
* Added json flag to base request and tests to check for JSON response

* Fixed a couple misspellings

* Returns body.results or body as a response. Added debug support. (#112)

* Now returning body.results or body as a response. Added debug support.

* Updated documents and examples

* Updating newly merged code

* Updated debug flag doc in README.md

* clean up from 1.x merge

* Implement promise support (#154)

* added promise support to base object

* updated sendingDomains for promise support

* updated inboundDomains for promise support

* updated messageEvents for promise support

* updated recipientLists for promise support

* updated relayWebhooks for promise support

* updated subaccounts for promise support

* updated suppressionList for promise support

* updated templates for promise support

* updated transmissions for promise support

* updated webhooks for promise support

* Replaced bluebird with native promise support for PR #154

* removed dependency for bluebird

* replaced bluebird on spec tests

* Switched JSHint for ESLint with SparkPost config (#159)

* Switched JSHint for ESLint with SparkPost config

* modified lint npm script to remove node_modules path

* Removed support for nodejs versions .10 & .12 (#157)

* Removed testing of nodejs versions .10 & .12, added lastest stable, 5, & 6

* latest is 6 so added 4

* reverting the npm version

* Switch to using npm scripts instead of grunt (#160)

* Remove coveralls task from grunt

* removed testing and coverage from grunt

* Removed unneed npm deps

* Updated docs to remove grunt references

* Added npm version scripts & removed Grunt

* Removed SendGrid Compatibility (#162)

* Set options.json=true for GET requests (#163)

* Setting options.json=true for GET requests

* Fixed tests

* Refactored transmissions library (#158)

* Refactored transmissions library & updated tests for promises

* merge flub, extra comma in package.json

* Updated transmission docs and examples

* Documentation updates

* Updated webhooks lib and tests

* Updated webhooks doc and examples

* updated tests for payloads and cloneDeep

* validates required parameters and jsdocs to webhooks wrapper

* update docs for webhooks

* updated docs, tests, and examples

* eslint fixes
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.

None yet

3 participants