kevinbeaty / any-promise

NOTE: You probably want native promises now
MIT License
179 stars 17 forks source link

Optional Import #15

Open jamestalmage opened 8 years ago

jamestalmage commented 8 years ago

An import that won't throw:

var Promise = require('any-promise/optional');
// => null if it can't be found, instead of throwing

We are doing it for any-observable: https://github.com/sindresorhus/any-observable/issues/12

It's probably got less utility here, as Promises are near ubiquitous

kevinbeaty commented 8 years ago

Yup. Maybe less utility, but probably worth doing here as well.

kevinbeaty commented 8 years ago

I may wait on this one for a bit. As you mentioned, there may not be much utility here and I'm hesitant to add something without an obvious benefit.

I'm a little concerned about reconciling the behavior between require('any-promise') and require('any-promise/optional') and describing the addition in the documentation, especially since require('any-promise') will rarely throw anyway as you mentioned.

The easiest implementation of optional would just wrap the require('any-promise') in a try/catch, but this would prevent late registration, which might be nice if one were using optional. As in "give me a promise if you have one, but I might check again later if you don't". But that might be over-thinking it.

jamestalmage commented 8 years ago

Hmm.

Late registration is an interesting discussion all on it's own.

That might be a whole new feature:

// late-registration.js
var register = require('./register');
var _registration = null;

function getRegistration() {
  if (_registration) {
    return _registration;
  }
  try {
    return  _registration = register();
  } catch (e) {
    return null;
  }
}

function LazyPromise(cb) {
  if (arguments.length !== 1) {
    // this is the standard, but protect against weird behavior 
    throw new Error('LazyPromise must be called with one and only one argument');
  }

  if (!getRegistration()) {
    throw new Error('No Promise implementation registered, yet LazyPromise was called');
  }

  if (!(this instanceof LazyPromise)) {
    throw new Error('LazyPromise must be called with new');
  }

  return new _registration.Promise(cb);
}

Object.defineProperty(module.exports, 'Promise', {
  enumerable: true,
  configurable: true,
  get: function () {
    return (getRegistration() && _registration.Promise) || LazyPromise
  }
});

Object.defineProperty(module.exports, 'implementation', {
  enumerable: true,
  configurable: true,
  get: function () {
    return (getRegistration() && _registration.implementation) || 'lazy'
  }
});

Object.defineProperty(module.exports, 'registered', {
  enumerable: true,
  configurable: true,
  get: function () {
    return Boolean(getRegistration());
  }
});

register would need to be modified somehow so it knew when you were making a "lazy" request. Otherwise the first lazy request would lock the implementation anyways for Node > 0.12

kevinbeaty commented 8 years ago

Late registration is an interesting discussion all on it's own.

Yeah. I'm not really sure I want to open that can of worms. I was going to say I didn't think it was possible but then I looked at your proposal closer. It's very clever. One potential problem is this wouldn't work for static methods on the Promise object for, e.g, bluebird's Promise.reduce, Promise.map, etc. (might be an interesting use case for Proxies, but I digress).

I think you are right that if we were to allow late registration it would make sense for both normal and optional require though.

The only reason I brought it up was because I tried to do it in my tests but I forgot how register worked. I should have read my documentation closer :)

kevinbeaty commented 8 years ago

But that might be over-thinking it.

I'm thinking too far down the hypothetical rabbit trail here which is the root cause of my hesitation. I'm already a bit concerned about the increasing size of the API. I'm starting to think it's time to slow it down a bit.

Back to optional. I'm thinking it would be a rare use case, and in those cases I'm not sure if this:

var Promise = require('any-promise/optional')
if(Promise){
   // do something
} else {
   // do something else
}

is all that much better than this:

try {
   var Promse = require('any-promise');
   // do something
} catch(e){
   // do something else
}

Or am I missing something?

jamestalmage commented 8 years ago

Or am I missing something?

If you are using the presence of a Promise implementation to feature toggle, you don't want to inline that try/catch if you have any performance concerns. A try/catch deoptimizes the entire containing function, not to mention that require('xxx') isn't free - even when the result is in the require cache.

So you'll probably end up recreating the optional behavior yourself:

The main use (I think) is for libraries that offer some functionality without a need for Promises, but might consider some sort of additional Promise support:

var Promise = require('any-promise/optional');

module.exports.mainFunctionality = function () {
 // works just fine without Promise
};

module.exports.needsAPromise = function () {
  // you wouldn't want to introduce a deoptimizing try/catch, or a require call here.
  if (!Promise) {
    throw new Error('no promise implementation, "needsAPromise" will not work');
  }
  // ...
};
kevinbeaty commented 8 years ago

Hmm. OK. Would this be a sufficient implementation of optional in your mind?

try {
   module.exports = require('.');  // require('any-promise')
} catch(e) {
   module.exports = null
}

If that's all it is, I can add it.

kevinbeaty commented 8 years ago

I've discovered a complication with this.

var Promise = require('any-promise/optional') // => null
// later
var Promise = require('any-promise')  // => {}

A similar things happens with two requires, but at least the first one throws

var Promise = require('any-promise') // throws
// later
var Promise = require('any-promise') // => {}

It seems that the empty module.exports is cached when requiring index throws the first time, and optional swallows this Error.

This version seems to behave a little better:

try { 
  module.exports = require('./register')().Promise || null
} catch(e) {
  module.exports = null
}
var Promise = require('any-promise/optional') // => null
// later
var Promise = require('any-promise')  //  throws (but only first time)
kevinbeaty commented 8 years ago

See related discussion in sindresorhus/any-observable#14