search

tfoxy / chrome-promise

Promises for chrome JavaScript APIs used in extensions and apps.
MIT License
150 stars 14 forks source link

Update to handle callbacks with multiple arguments. #6

Closed riggs closed 8 years ago

riggs commented 8 years ago

Also, move from using arguments to the spread operator to be more explicit.

tfoxy commented 8 years ago

Hi. Thanks fo the PR. There is an error in the tests. self is not defined. I think the error should be solved if you rename thisArg to self.

If you fix the error, I can merge the changes.

I also like the solution proposed by @amio, as it has a more predictable behaviour. I will add an option to the constructor so any of the two solutions can be used.

riggs commented 8 years ago

Hmm, strange, it looks like the version pushed to npm is a different version of the codebase, ever so slightly. I'll update my changes to match the current version.

riggs commented 8 years ago

It looks like the current failure is because you're testing against a version of Chrome from 2014:

Session info: chrome=37.0.2062.120

The spread operator has been supported since Chrome 46.

tfoxy commented 8 years ago

I will try to update the chrome version used by travis by using trusty.

tfoxy commented 8 years ago

I updated chrome to the current stable version. Can you rebase the PR?

riggs commented 8 years ago

Done.

tfoxy commented 8 years ago

Merged! I will release the new version tomorrow.

riggs commented 8 years ago

Awesome! Thanks so much!

riggs commented 8 years ago

BTW, between your library and the babel async-to-generator transform plugin, I'm successfully using async/await wrapped chrome APIs, which is such a huge improvement over callback-hell.

tfoxy commented 8 years ago

Yeah, it's a shame that the api does not have promises by default. It was very common for me to have multiple callbacks. With async/await syntax I would think it's even simpler. And it's not necessary to use Q or Bluebird for that.

v2 is now live. Thanks again for the PR.