Remove Q Dependency #456

Aedalus · 2018-11-06T00:50:26Z

Summary:

This PR removes the dependency on Q while maintaining the current promise and callback based API.

Code sample:

Schema

// Code here

Model

// Code here

General

// Code here

GitHub linked issue: #216

Closes #216

Type (select 1):

Bug fix
Feature implementation
Documentation improvement
Testing improvement

Test added to report bug (GitHub issue #--- @---)
Something not listed here

Is this a breaking change? (select 1):

🚨 YES 🚨
No
I'm not sure

Is this ready to be merged into Dynamoose? (select 1):

Yes
No

Are all the tests currently passing on this PR? (select 1):

Yes
No

Other:

I have searched through the GitHub pull requests to ensure this PR has not already been submitted
I have updated the Dynamoose documentation (if required) given the changes I made
I have added/updated the Dynamoose test cases (if required) given the changes I made
I have run npm test from the root of the project directory to ensure all tests continue to pass
I agree that all changes made in this pull request may be distributed and are made available in accordance with the Dynamoose license
All of my commits and commit messages are detailed, explain what changes were made, and are easy to follow and understand
I have confirmed that all my code changes are indented properly using 2 spaces
I have filled out all fields above

coveralls · 2018-11-06T00:53:26Z

Pull Request Test Coverage Report for Build 709

716 of 832 (86.06%) changed or added relevant lines in 5 files are covered.
5 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.4%) to 85.55%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/Query.js	114	121	94.21%
lib/Scan.js	133	146	91.1%
lib/Table.js	106	149	71.14%
lib/Model.js	359	412	87.14%

Files with Coverage Reduction	New Missed Lines	%
lib/Model.js	5	84.1%

Totals
Change from base Build 700:	0.4%
Covered Lines:	1878
Relevant Lines:	2123

💛 - Coveralls

Aedalus · 2018-11-06T01:12:31Z

I'll look into the code coverage over the next couple days as I find some time.

fishcharlie

First of all. AMAZING job!! I think you touched almost every line in this package. If only Git worked better and only showed the CHANGES it would have been a lot easier to review haha.

A lot of my comments are duplicates of ones I made previously. I tend to do that so it's easy to go back later and ensure that it was fixed in all the spots. So that is the purpose for me making so many notes.

I did have some major questions about high level things that we are doing in this PR. But a lot of those comments are duplicates of the first one.

There is also one comment I made with more detail that references one higher up. So it'd probably be a good idea to read through all the comments before making any changes.

HUGE thank you for this! 🎉 🎆 👏 🥇

test/Model.js

lib/Model.js

fishcharlie · 2018-11-06T01:01:27Z

lib/Model.js

-    if(options.overwrite === null || options.overwrite === undefined) {
-      options.overwrite = true;
-    }
+  if(next){


Instead of doing this. Why not create a helper function to use throughout the codebase? For example.

function promisify(promise, cb) { promise.then(result => { cb(null, result); }, err => { next(err); }); return promise; }

Then instead of this if statement. We can just set the promise to a variable instead of returning it. Then do something like:

return promisify(promise, next);

That way we don't have to write the then logic and all of that over and over.

You might have to play with that code. Probably do need a if statement within that function I created to ensure that cb exists. Not sure if the naming works and everything like that. So might need to play with it quite a bit.

With the current implementation I think someone trying to combine a callback with a promise would fail. So something like:

const item = Model.get("myitem", function(err, result) { console.log(result); });

I think in that case item is undefined with your changes. But I think in the current version item would exist.

I would almost consider that a breaking change. I have no idea how people are using Dynamoose, and although I wouldn't recommend that, I can't be positive that people aren't writing code like that.

I think my suggestion above would address that.

So this is something I definitely struggled with, and would be up for going a different direction.

If we return a promise and the User doesn't catch it, Node will throw an unhandled promise rejection. Q promises don't dispatch the same node event, so they just fail silently. So we can return the promise, but if they are only using a callback Node will write to std err, which hopefully would never break anything, but would at least likely raise a few eyebrows to whether something changed. I feel like failing silently definitely isn't the way to go though. If you have any ideas for how you want to handle it, let me know!

Definitely up for abstracting it with the above though. I'll try to implement the above. Unfortunately the way some functions dynamically determine if a param is a config or a callback makes it pretty difficult to do the abstraction outside the function. Would definitely make the code cleaner if support was at least dropped for passing in a cb if omitting options, but that would definitely be a breaking change.

Thinking on the above, is the plan for the above to continue to support callbacks, or eventually move to entirely promises? Would it make sense to start deprecating certain things like callbacks, or at least passing callbacks in place of options? Might make any eventual breaking changes to the API a lot easier, but I don't know if that's the eventual plan.

Using my first example above. Wouldn't we be catching the error there tho? Isn't the only difference the fact that we are returning the promise and abstracting it a bit more?

As of right now there is no plan to deprecate callbacks. It's something I'd be open to discussing. But as of right now there has been no discussion regarding it.

This should be abstracted into it's own file/method now. I've called it linkPromiseToCallback, and put it in a new helpers.js file. Not in love with either though, so if you want me to change the name/location let me know.

lib/Table.js

fishcharlie · 2018-11-06T01:38:14Z

lib/Table.js

 };

 Table.prototype.update = function (next) {
  // let ddb = this.base.ddb();
  // ddb.updateTable();
-  const deferred = Q.defer();
-  deferred.reject(new Error('TODO'));


I wonder who wrote this code, and what the purpose of this is. Before this gets merged in, I should probably figure out how to look at the blame, to figure out when this was written to track it down and figure out how to implement it.

If you are able to do that quickly that'd be awesome. But I think it needs to be done before this gets merged in, or else I think it'd be a lot harder afterwards.

fishcharlie · 2018-11-06T01:40:09Z

package.json

@@ -50,8 +50,7 @@
    "debug": "^2.6.8",
    "deep-equal": "^1.0.1",
    "hooks": "0.3.2",
-    "object-path": "^0.11.4",
-    "q": "^1.5.0"


fishcharlie · 2018-11-06T01:46:58Z

@Aedalus One other question. Do you think we should add tests for this? I mentioned that we shouldn't change the tests in this PR. But I'd totally be in for adding tests.

fishcharlie · 2018-11-06T01:49:36Z

@Aedalus And I just saw your comment about code coverage. It's been doing that for a while now. I feel like it's some type of glitch. With all the changes you made, and no changes to the tests, losing 0.2% coverage makes 0 sense to me. So if you want to take a look at it, feel free. But it's probably not related to this PR, and I'll merge regardless of that. If you want to look at it feel free to open a separate PR tho!

fishcharlie · 2018-11-06T18:21:17Z

Looks like test failed due to timeout. Restarting.

fishcharlie · 2018-11-06T18:22:31Z

@Aedalus If you could do me a favor and mark the comments I made as resolved once you finish them I'd really appreciate it. Just so we can keep track of what changes still need to be addressed before merging. I marked a few as complete based on the last commit you made.

fishcharlie · 2018-11-07T02:15:06Z

@Aedalus Looks like this build is failing for some reason. Wanna take a look at that at some point?

fishcharlie · 2018-11-12T17:34:45Z

@Aedalus Were you able to make any progress on this?

Aedalus · 2018-11-14T14:09:38Z

@fishcharlie I've made some, work's just been a little crazy. Hoping to finish it up by this weekend so the PR isn't just rotting here.

Co-Authored-By: Aedalus <TheAlexanderHiggins@gmail.com>

Aedalus · 2018-11-17T09:46:00Z

@fishcharlie Newest changes are in. I tried to follow your suggestion with removing the if(next) statements, and abstracting it into another function. Let me know if you see anything off.

fishcharlie · 2018-11-18T17:46:15Z

lib/Model.js

@@ -276,75 +276,78 @@ function validKeyValue(value) {
  return value !== undefined && value !== null && value !== '';
 }

-Model.prototype.put = function(options, next) {
+Model.prototype.put = function(options = {}, next) {


The diff of this entire section looks really different. For example, the if(typeof options === 'function') { is within the switch statement. I think the goal of this PR should be focused entirely on removing the Q dependency, and not changing any logic at all. The goal should be to create the least amount of changes possible to get this working.

These changes look ok, but I think they should be made in a separate PR from this.

fishcharlie · 2018-11-19T20:23:07Z

lib/Model.js

-    RequestItems: {}
-  };
+  return new Promise((resolve, reject) => {
+    debug('BatchGet %j', keys);


This still hasn't been addressed

fishcharlie · 2018-11-19T20:25:48Z

lib/helpers.js

+
+module.exports = {
+  linkPromiseToCallback,
+}


This should have a new line at the end of the file

fishcharlie · 2018-11-19T20:27:08Z

lib/helpers.js

@@ -0,0 +1,11 @@
+function linkPromiseToCallback(promise, cb) {
+  // If a callback exists, call it
+  if(cb) promise.then(result => { cb(null, result); }, err => { cb(err); });


Can we separate this out into new lines?

I'm also concerned about this. We need to make sure that it falls through when you chain stuff to this promise. Have you tested promise support manually with this?

fishcharlie · 2018-11-29T22:01:57Z

Another issue I just realized.

https://github.com/fishcharlie/dynamoose/blob/pluginsupport-async/lib/Scan.js#L207

That line. We are passing deferredMain in, so that will cause problems. So we will need to make sure that works fine.

fishcharlie · 2018-11-30T20:45:35Z

@Aedalus Any progress on this? 😃

fishcharlie · 2019-02-04T06:34:24Z

Closing this due to no recent activity. If this is still a problem or you have questions please feel free to comment again and we can try to help further.

Aedalus added 2 commits November 1, 2018 20:38

Removed Q dependency

c90f537

Updated model

0dde707

Aedalus mentioned this pull request Nov 6, 2018

Support for native promises (async/await) #216

Closed

fishcharlie requested changes Nov 6, 2018

View reviewed changes

Removed debugging code. Commented back in console logs in tests

2429e3d

Aedalus and others added 5 commits November 17, 2018 04:28

Added linkPromiseToCallback function

ccf0736

Update lib/Model.js

378ebec

Co-Authored-By: Aedalus <TheAlexanderHiggins@gmail.com>

Update lib/Model.js

0a180d3

Co-Authored-By: Aedalus <TheAlexanderHiggins@gmail.com>

Fixed debug statement in Model and rolled package lock back to trunk

6c48136

Merge branch 'master' of https://github.com/Aedalus/dynamoose

e9e5ac8

fishcharlie reviewed Nov 19, 2018

View reviewed changes

mogwai mentioned this pull request Nov 27, 2018

Plugin Support - BETA #325

Merged

fishcharlie mentioned this pull request Nov 29, 2018

Caching using LRU or Memcached #335

Closed

fishcharlie added the status:do-not-merge label Dec 1, 2018

fishcharlie closed this Feb 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Q Dependency #456

Remove Q Dependency #456

Aedalus commented Nov 6, 2018

coveralls commented Nov 6, 2018 •

edited

Aedalus commented Nov 6, 2018

fishcharlie left a comment

fishcharlie Nov 6, 2018

Aedalus Nov 6, 2018

Aedalus Nov 6, 2018

fishcharlie Nov 6, 2018

Aedalus Nov 17, 2018

fishcharlie Nov 6, 2018

fishcharlie Nov 6, 2018

fishcharlie commented Nov 6, 2018

fishcharlie commented Nov 6, 2018

fishcharlie commented Nov 6, 2018

fishcharlie commented Nov 6, 2018

fishcharlie commented Nov 7, 2018

fishcharlie commented Nov 12, 2018

Aedalus commented Nov 14, 2018

Aedalus commented Nov 17, 2018

fishcharlie Nov 18, 2018

fishcharlie Nov 19, 2018

fishcharlie Nov 19, 2018

fishcharlie Nov 19, 2018

fishcharlie commented Nov 29, 2018

fishcharlie commented Nov 30, 2018

fishcharlie commented Feb 4, 2019

Remove Q Dependency #456

Remove Q Dependency #456

Conversation

Aedalus commented Nov 6, 2018

Summary:

Code sample:

Schema

Model

General

GitHub linked issue: #216

Type (select 1):

Is this a breaking change? (select 1):

Is this ready to be merged into Dynamoose? (select 1):

Are all the tests currently passing on this PR? (select 1):

Other:

coveralls commented Nov 6, 2018 • edited

Pull Request Test Coverage Report for Build 709

💛 - Coveralls

Aedalus commented Nov 6, 2018

fishcharlie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishcharlie commented Nov 6, 2018

fishcharlie commented Nov 6, 2018

fishcharlie commented Nov 6, 2018

fishcharlie commented Nov 6, 2018

fishcharlie commented Nov 7, 2018

fishcharlie commented Nov 12, 2018

Aedalus commented Nov 14, 2018

Aedalus commented Nov 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishcharlie commented Nov 29, 2018

fishcharlie commented Nov 30, 2018

fishcharlie commented Feb 4, 2019

coveralls commented Nov 6, 2018 •

edited