thlorenz / proxyquireify

browserify >= v2 version of proxyquire. Mocks out browserify's require to allow stubbing out dependencies while testing.
MIT License
152 stars 24 forks source link

Avoid using unsupported methods #28

Closed roberttod closed 9 years ago

roberttod commented 9 years ago

Object.keys and Array.prototype.map are not supported in older browsers (IE8 and lower)

bendrucker commented 9 years ago

Will let @thlorenz have the final word here but I think it's perfectly reasonable in 2015 to assume that people who need to support older browsers will use an ES5 polyfill.

roberttod commented 9 years ago

Since this is a testing tool, it would be great if it worked seamlessly in IE8!

I am using this to test an npm package so a polyfill doesn't really work for me

bendrucker commented 9 years ago

Including it in the source of an npm package is a definitely a bad idea, no questions there. And yes, requiring a polyfill does mean that it's tricky for you're basically assuming that your users are supplying a polyfill as well, at least as far as the tests are concerned. I think it's fair to start insisting people do that in order to support a browser that's less than a year from being end-of-lifed in order to write code that's better for everyone else.

roberttod commented 9 years ago

I appreciate your sentiment, there is nothing I would like to do more than drop IE8 support. Unfortunately I am building a tool that will track browser usage share among other things so I don't really have an option.

For me, the only alternative is to fork the repo and publish it to npm under a different name. I would like to avoid this if possible so I will wait for @thlorenz to weigh in before going further.

I would agree with you that this bloats the code for no good reason however since it is a testing tool I think it is a reasonable patch. That being said, I think proxyquireify is pretty awesome, it would be a shame to have to use a fork every time I want to use it!

thlorenz commented 9 years ago

Unfortunately I am building a tool that will track browser usage share among other things so I don't really have an option.

You couldn't add a polyfill to your tests?

If you give me a use case which cannot be fixed with a polyfill I'm actually open to adding this feature to proxyquireify. I'm just not convinced that you couldn't just require('some-polyfill-module') at the top of your test which then fills in the missing methods before proxyquireify uses them.

Here is an example:

require('es5-shim');
require('es5-sham');
roberttod commented 9 years ago

I could polyfill the test however this would defeat the point of running the test on older browsers e.g. if I was to use a method like Object.keys in my code, the test would pass despite it being unsupported.

thlorenz commented 9 years ago

Why don't you just run those tests, then polyfill before running the tests that depend on proxyquierify?

Yours is a very unique use case and since a workaround exists, even for your case I'm hesitant to change this module for it.

However you're free to publish a proxyquireify-es3 in case you're not satisfied with the work around.

roberttod commented 9 years ago

I can't really afford to test any part of the code with a polyfill as it would be wrong to claim IE8 support if I did so. I have used this module multiple times in the project (because it seems to be by far the best way to do dependency injection).

If you don't want to merge, I will go ahead with a fork. This library has been pretty helpful so keep up the good work :+1:.

bendrucker commented 9 years ago

@roberttod Just a heads up that proxyquireify will be adding more ES5 code soon (Object.defineProperty), see https://github.com/thlorenz/proxyquire/issues/64#issuecomment-103401453. I'll ship an ES3-compatible method in the key filling lib that you can use in your fork.

roberttod commented 9 years ago

Thanks!