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

chai and jquery as peerDependencies #40

Open
jmendiara opened this issue Feb 21, 2014 · 6 comments
Open

chai and jquery as peerDependencies #40

jmendiara opened this issue Feb 21, 2014 · 6 comments

Comments

@jmendiara
Copy link

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link

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
Copy link

eeroan commented Apr 21, 2015

The pull request #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.

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

No branches or pull requests

4 participants