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

Added the Promise API to the Glob prototype #243

Closed
wants to merge 2 commits into from
Closed

Added the Promise API to the Glob prototype #243

wants to merge 2 commits into from

Conversation

thaggie
Copy link

@thaggie thaggie commented Jan 16, 2016

I've added the Promise API to Glob objects, this is useful for keeping the indentation flat when doing further I/O operations on the returned matches.

glob('**/*.js')
.then(function(matches) {
  // do something
})
.catch(function(error) {
  // handle the error
})

…JavaScript/Reference/Global_Objects/Promise#Methods_2) to the `Glob` prototype, this is useful for keeping the indentation flat when doing further I/O operations on the returned matches.

```
glob('**/*.js')
.then(function(matches) {
  // do something
})
.catch(function(error) {
  // handle the error
})
```
@isaacs
Copy link
Owner

isaacs commented Jan 16, 2016

I'm not opposed to this in principle, however:

  1. This should be a standalone module that can promisify a thing given an EventEmitter and a "resolution" event (with "error" considered the "rejected" event, of course). Flow-control boilerplate doesn't belong in this module.
  2. That module should work on Nodes less than 0.12 (I'm told Bluebird is the thing all the cool kids use these days for their cross-platform promises.) That's what's breaking the Travis-CI test.
  3. It seems like this isn't matching the Promises/A+ spec, which says that .catch() should behave quite a bit differently from how it's implemented here.

Another approach would be, rather than tacking .then() and .catch() onto the Glob prototype, just create a glob.promise(pattern, [options]) => Promise method, since that's really what you'd probably want to use anyway, and that's pretty trivial. Might look something like this:

glob.promise = function (pattern, options) {
  return new Promise(function (resolve, reject) {
    var g = new Glob(pattern, options)
    g.once('end', resolve)
    g.once('error', reject)
  })
}

@thaggie
Copy link
Author

thaggie commented Jan 18, 2016

The glob.promise route is a little tedious (for consumers) but probably the cleanest one (not hacking a class to duck type to a Promise).

Personally I prefer things not to drag in dependencies and so I'd prefer that when you use a system without promises you just don't get promises - this is the way I implemented the original but not for the tests as I had naively assumed the CI would be just using a modern system.

event-to-promise uses any-event which detects whatever promise library the system has installed and uses that.

With regards to the non-conformance to the A+ spec - must admit I didn't look at it till now - just looked at the MDN description - I can't see any mention of catch.

@isaacs
Copy link
Owner

isaacs commented Jan 18, 2016

Another route is just to promise up the (lower case) glob() function of a callback isn't provided, but if that returns a Glob instance today, it'd be a breaking change.

@popomore
Copy link

popomore commented Feb 9, 2017

Maybe you can use https://github.com/node-modules/mz-modules

@thaggie thaggie closed this Jul 17, 2018
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