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

Update npm dependencies #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adbutterfield
Copy link

Great package you have here, but I noticed the dependencies are out of date.

I ran the test script, and everything is still green.

@dalemartyn
Copy link

you might want to check the 'del' dependancy. it changed to a promise API, so it might break the callback on line 33. Tests will pass as the tests use a different delFn.

@callumacrae
Copy link
Owner

Hey. Happy to accept this PR (except for the note above), but there's a conflict now.

@adbutterfield
Copy link
Author

Ah, I totally forgot about this. One of my goals this year is to start contributing back to open source projects I use.

I fixed just the conflict, and I'll look at the other issue today!

@adbutterfield
Copy link
Author

I implemented a change where delFn is always a promise. Not sure if it's a naive fix or not.

Also I noticed the syntax wasn't consistent, so I beautified the code.

@adbutterfield
Copy link
Author

I also tried it in a project I'm working on right now, and it's working with no problems.

@callumacrae
Copy link
Owner

I totally forgot about this too - my bad!

Nice, happy with this except for one thing: not really happy adding bluebird as a dependency just to promisify a function that is only really used during testing.

Two alternatives:

  • Manually wrap deleteFn with a native promise and update the version required
  • Just turn deleteFn into a promise to match del. It isn't documented, so it's fine to change this

Both are breaking changes, but I'm happy not supporting Node 0.10 anymore.

@adbutterfield
Copy link
Author

Yeah, I took the lazy route and just used bluebird.

Is there an easy way to turn a non-promise into a promise with the native Promise? I've actually only used bluebird for that sort of thing.

Or, if you want to change it so that delFn option has to be a promise, I could just remove bluebird. Then delFn in the tests would have to be updated to be promises I guess?

@callumacrae
Copy link
Owner

function promisify(fn) {
  return new Promise(function (resolve) {
    fn(resolve);
  });
}

Untested, but something like that should work. Could also wrap it in a try-catch block for the reject function, but I don't think that's necessary in this case.

@adbutterfield
Copy link
Author

That didn't seem to work.

I did come across this package though, and this solution seems to work:
https://github.com/jcollado/promisify-function/blob/master/src/index.js

Maybe could just add it as a dev dependency?

@callumacrae
Copy link
Owner

Ah what I wrote isn't a promisify function, it's what you would return as delFn - ignore that!

This should work:

function promisify(fn) {
  return function () {
    var args = Array.prototype.slice.call(arguments);
    return new Promise(function (resolve) {
      fn.apply(null, args.concat(resolve));
    });
  };
}

If that doesn't work just go ahead and add the dependency :)

@adbutterfield
Copy link
Author

That one didn't quite do it either. I swapped out bluebird for that promisify-function package.

@adbutterfield
Copy link
Author

Oh also, I just realized, that promisify-function package uses babel to transpile for compatibility back to only node 4.

@callumacrae
Copy link
Owner

Looks good now! I've updated the travis versions on master, could you rebase onto master and then when the tests are merged I'll merge this.

Thanks!

@adbutterfield adbutterfield force-pushed the update-npm-dependencies branch from 8f8036c to ddd4868 Compare March 11, 2017 13:21
@adbutterfield adbutterfield force-pushed the update-npm-dependencies branch from ddd4868 to 5d5d6e8 Compare March 11, 2017 13:25
@adbutterfield
Copy link
Author

So I did a squash rebase on the branch. The things I did in this PR, in case you want to add it to the release notes:

  • Update npm dependencies
  • Beautify index.js for consistent syntax
  • Change call to del to use promise rather than callback
  • Wrapped custom delFn with promise using promisify-function package

@jakezatecky jakezatecky mentioned this pull request Jan 22, 2019
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