chaijs / chai-jquery

jQuery assertions for chai
MIT License
372 stars 69 forks source link

chai and jquery as peerDependencies #40

Open jmendiara opened 10 years ago

jmendiara commented 10 years ago

Have you consider using

"devDependencies": {
  "mocha": "1",
  "mocha-phantomjs": "3"
}, 
"peerDependencies": {
  "chai": "1",
  "jquery": "2"
}

To enable npm to manage your dependencies, and share the dependencies lifecicle with other peer projects (like karma plugins)?

jfirebaugh commented 10 years ago

Definitely chai makes sense as a peer dependency. Not sure jQuery does though -- it should work with pretty much any jQuery version, so there's not really anything for peer dependencies to enforce.

jmendiara commented 10 years ago

https://github.com/chaijs/chai-jquery/blob/master/chai-jquery.js#L8 https://github.com/chaijs/chai-jquery/blob/master/chai-jquery.js#L16

If you need jquery to run, then jquery is a dependency. If you are using jquery as a collaborator, then is a peerDependency

In the node scenario, with your actual code:

var chai = require('chai'),
    jquery = require('jquery'),
    utils = {.....}
    chaiJquery = require('chai-jquery')(chai, utils, jquery);

You are delegating to the node developer to install both jquery and chai (strong dependencies) in the app, and pass it to your factory. Therefore, in the internals chai-jquery is not using a jquery dependency: you do not have a require('jquery')in your code.

IMHO, a better approach can be let jquery-chai to use the its peerDependencies (with version: "*" for jquery) and simplify your node API to

    chaiJquery = require('chai-jquery')(utils);

in your code...

module.exports = function(utils) {
   return chaiJquery(require('chai'), utils, require('jquery'));
};

taking the user installed dependencies. NPM will install them if they are not present.

This is exactly the same behaviour that happens in the browser: Your code assumes jquery and chai have been included in the dom, With this approach you will have a consistent API between the 3 flavors: Node, AMD, and script, because the package manager has taken the responsibility of managing deps

With jquery as peer dependency, the user will use the same jquery version automatically in both their test (via chai-jquery) and its own code

WDYT?

jfirebaugh commented 10 years ago

Yeah, it's true that the node API is currently a second class citizen. The standard convention for chai plugins is to export a function which can be passed to chai.use(...), so I'd want to keep the first and second arguments as they are now. But it sounds like peerDependencies can eliminate the last argument, which is good.

On the other hand, some people want to be able to specify the jQuery object (#36), so they can pass Ember.$ or (I imagine), zepto.

On the third hand, the wrapper hard-codes a jQuery dependency for AMD already.

So it's a bit of a mess and I'm not entirely sure what the best way of satisfying everybody is.

jmendiara commented 10 years ago

There is no good solution, there is the least bad solution

The least bad solution is up to you ;) But, now makes sense to update your dependencies? If you are not going to require nothing in the node side, makes no sense to have *dependencies in the package.json. But if you are going to add require... please, make it as a peerDependencies

brian-mann commented 10 years ago

I have a situation where I also would like to specify the jQuery object -- I'm juggling between a parent and a child iframe and there is a strong possibility of 2 different jQuery objects - and instanceof hard codes the object to only one jQuery function.

eeroan commented 9 years ago

The pull request https://github.com/chaijs/chai-jquery/pull/66 I just made fixes issues with instanceof checks. It's still backward compatible, but I would like to get rid of the magic for good.