Allow custom implementation of Promise

[tnguyen14]

This fixes #11, providing support for browser with no Promise implementation by allowing application to pass in its own implementation of Promise.

This is done to overcome the limitation of browserify only allowing static requires and not dynamic one (which has been used up to now in the Node.js version)

Thank you Kevin for providing the code sample and ideas to start this off. I have also implemented the option to bypass global registration.

I removed register-shim.js and just handled the node vs. browser switching inside register.js instead of relying on the browser field. This is done to reduce the duplication between the two.

One question I have is should the naming of opts.Promise be changed to opts.impl to be more descriptive that you're passing in the actual implementation of the Promise?

One todo item is to update the README with the new opts. I can do that once the changes are approved.

/cc @kevinbeaty

[kevinbeaty]

@tnguyen14 This looks great on first skim. I'll have to review in more depth, but it's on the right track.

I removed register-shim.js and just handled the node vs. browser switching inside register.js instead of relying on the browser field. This is done to reduce the duplication between the two

I think this is fine, as long as browserify and webpack ignore the calls to bare require(lib) in loadImplementation. That is the reason I split it out in the first place, but maybe more recent versions allow this.

One question I have is should the naming of opts.Promise be changed to opts.impl to be more descriptive that you're passing in the actual implementation of the Promise?

Yeah, I was considering this too but I already used 'implementation' for the name of the implementation (e.g. 'bluebird'). I think calling it Promise might make sense so it is obvious you need to pass a Promise constructor. I'd be up for other names, but 'implementation' has already been used.

One todo item is to update the README with the new opts. I can do that once the changes are approved.

This would be great. Also, any idea on how to test the browser version?

Thanks so much for your help.

[tnguyen14]

I think this is fine, as long as browserify and webpack ignore the calls to bare require(lib) in loadImplementation.

I just added a check to only do the loadImplementation in Node.

I think calling it Promise might make sense so it is obvious you need to pass a Promise constructor. I'd be up for other names, but 'implementation' has already been used.

I agree with you on the fact that the implementation name is already used. It was kinda the reason I was confused by it in the first name, because it actually meant the name of the implementation package, not the actual implementation. I'm okay with just keeping it Promise.

Also, any idea on how to test the browser version?

I was thinking about this as well. I was gonna add a simple test using zuul and phantom to demonstrate the use case. I'm just not familiar with Makefile, so I am not sure how to instrument it right. I'll read up on it a bit more. The other question is how to integrate it with Travis CI, i.e. whether that should be run for each of the Node environment that is being done right now? Seems unnecessary to me, but that might be the simplest solution.

[kevinbeaty]

I just added a check to only do the loadImplementation in Node.

Does browserify ignore the code in the if(isNode) checks? I just don't want to include, e.g., the process shim in the browser version when it is browserified. I think at one point, browserify threw an error because of loadImplementation, but I might be mistaken.

I was thinking about this as well. I was gonna add a simple test using zuul and phantom to demonstrate the use case.

I think it would be awesome if you used zuul and phantom for the browser tests.

The other question is how to integrate it with Travis CI,

At one point, I considered adding a separate github repository that just had browser tests so it could be managed separately. That way the zuul and Travis stuff could be maintained in whatever way is easiest without worrying about integrating the other tests.

i.e. whether that should be run for each of the Node environment that is being done right now?

I don't expect very many changes to any-promise because of limited scope, so it would be fine this way as well. As far as the Makefile, you should be able just to add a line for your zuul tests underneath the test target, e.g.:

test: | node_modules
    `npm bin`/mocha test/tests.js
     `npm bin`/zuul test/browser-tests.js

I'm not too tied to using the Makefile either. I'd be OK with adding the commands to package.json. What do you normally do?

[kevinbeaty]

I just had another idea re. testing. Can we just add another env for zuul and run it programatically in tests.js? Something like:

if(process.env.ANY_PROMISE){
  // should load registration regardless
  expectedImpl = process.env.ANY_PROMISE
   if(expectedImpl === 'zuul-tests'){
        // whatever you need to run zuul
        zuul('./browser-tests.js')
        return;
   }
  require('../register')(expectedImpl)
} else {
... same as before

[tnguyen14]

Does browserify ignore the code in the if(isNode) checks?

Oh you're right. I forgot to test this case. It seems like isNode is true for browserify. So I think we can do this 2 ways. (1) Add an addition check for process.browser is true, or (2) go back to the use of register-shim.js, but use some shared code to reduce the duplication, which is the original goal.

I'm not too tied to using the Makefile either. I'd be OK with adding the commands to package.json. What do you normally do?

I've been using npm run scripts in package.json mostly, and it seems to be working well and readable.

I really like the idea of setting process.env.ANY_PROMISE to zuul to run that test as well. And it doesn't require any change to the use of Makefile. I will add that next.

[kevinbeaty]

So I think we can do this 2 ways. (1) Add an addition check for process.browser is true, or (2) go back to the use of register-shim.js, but use some shared code to reduce the duplication

I'd rather not include the process or global shim in the browser version and keep it as small as possible. So I think that means I prefer Option 2. I've never used the process.browser flag before though, so if using that eliminates the dead code and does not load the entire process shim, that would be OK with me too.

[tnguyen14]

So it took me a while, but I think I just got some browser testing in and working. Please review and let me know.

The remaining thing will be to fix the issue with isNode as discussed above.

[tnguyen14]

I abstracted register.js out to lib/index.js, and then have register.js and register-shim.js to call lib with a boolean parameter for isBrowser. I changed from isNode to isBrowser so that the default behavior is node.

[kevinbeaty]

This looks good. Thank you. I tweaked it a bit so that Node.js specific stuff remains in register.js and browser specific stuff remains in register-shim.js . That way we won't get the process or global shim or the Node specific stuff in the browserified version. See https://github.com/kevinbeaty/any-promise/commit/8e12f95f43e4576ae2bf50a5ddff3fb263f0abc7

This is committed on my custom-promise-impl branch. If it looks good to you, you should be able to pull it to your branch and push again to update this PR. If that works, I think the only remaining thing would be to update documentation. (I can do this when I publish if you would prefer, just let me know).

[tnguyen14]

It looks good to me. I've done that.

Please feel free to update the documentation as you see fit.

[kevinbeaty]

Thanks! I'll try to get this published soon.

[kevinbeaty]

Published in v1.2.0