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

Add Promise support #12

Merged
merged 4 commits into from Sep 30, 2019
Merged

Add Promise support #12

merged 4 commits into from Sep 30, 2019

Conversation

urish
Copy link
Contributor

@urish urish commented Sep 27, 2019

  • Maintain backward compatibility by still supporting callbacks
  • Should work with existing code that uses the promisify wrapper

@guymcswain
Copy link
Owner

@guymcswain guymcswain commented Sep 27, 2019

Please replace the readme usage example with your 'blinkwave' usage example, using the new promise support and waveChainTx API update. I think the new usage example will not only embody the essence of the pigpio library - hardware controlled/software managed gpio - but demonstrate the new API support included in this release. Please bump the version to 1.3.0 in the pigpio info object.

I'm away from my environment right now but will get to test the new code sometime early next week.

@urish
Copy link
Contributor Author

@urish urish commented Sep 27, 2019

Alright, I updated the usage example / bumped version number. Thanks again for creating and maintaining this library!

@guymcswain
Copy link
Owner

@guymcswain guymcswain commented Sep 30, 2019

@urish , I have successfully run my regression test on the promises branch with the following modifications:

  1. The result argument to the callback function of the request promise should be ...result. This will capture arguments that are returned from extended commands - such as gpio.serialRead().

  2. The originalCallback() invocation is return'ed. I need your help on this. I stumbled on this trying to find a way to quiet the 'UnhandledPromiseRejectionWarning' that is issued when an error is returned by the request api.

The complete promise is summarized here:

var promise = new Promise(function (resolve, reject) {
      var originalCallback = cb
      cb = function(error, ...result) {
        if (typeof originalCallback === 'function') {
          return originalCallback(error, ...result)
        }
        if (error) {
          reject(error)
        } else {
          resolve(...result)
        }
      }
    })

Finally, waitNotBusy's promise needs to include a check to verify that cb is a function.  I cant' recall why I had a call to this api void of arguments but nevertheless it is there in the regression test.

I'm ready to accept this PR with these changes if you can provide some assurances that they (ie #2) won't break the behavior expected by the promise.

@urish
Copy link
Contributor Author

@urish urish commented Sep 30, 2019

Hi @guymcswain !

  1. resolve only takes a single argument. In this case, we can check how many arguments that callback has received, and resolve with in array in case of multiple arguments (such as gpio.serialRead())

  2. I'm not sure what is the purpose of returning the original callback from the handler - what it basically does is never resolving/rejecting the promise. In other words, if I await and pass a callback, the code after the await will never execute. The UnhandledPromiseRejectionWarning is there for a reason, but a workaround I can suggest is simply not returning a promise if a callback was provided. So you get to choose: either callback or a promise. Should I go ahead and make this change?

  3. Are you talking about waveNotBusy? sure, we could easily do that

@guymcswain
Copy link
Owner

@guymcswain guymcswain commented Sep 30, 2019

Yes, I believe the changes you propose would satisfy my legacy code.

1. Return a promise only when callback is not provided,
so legacy code doesn't get `UnhandledPromiseRejectionWarning`
2. Support methods that return multiple values
3. Support calling waitNotBusy() without a callback
@urish
Copy link
Contributor Author

@urish urish commented Sep 30, 2019

Alright, here we go!

@guymcswain guymcswain merged commit 6656d31 into guymcswain:master Sep 30, 2019
@guymcswain
Copy link
Owner

@guymcswain guymcswain commented Sep 30, 2019

Thank you @urish for this nice contribution to pigpio-client. My regression test passes with and without the util.promisify wrapper! I'll be making some updates to the regression then publish 1.3.0 to npm thereafter.

@urish
Copy link
Contributor Author

@urish urish commented Sep 30, 2019

Lovely, pleasure working together on this mini-project :)

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

2 participants