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

Installing should package #22

Closed
nunorafaelrocha opened this issue Feb 21, 2016 · 5 comments
Closed

Installing should package #22

nunorafaelrocha opened this issue Feb 21, 2016 · 5 comments

Comments

@nunorafaelrocha
Copy link
Collaborator

Since should package is registered as a devDependency and peerDependency, anyone that follows the instructions and isn't using should runs into an UNMET PEER DEPENDENCY warning followed by a Cannot find module 'should' error when running jscs.

The package is used in the requireShouldAssertionExecution rule, so I think we have two options:

  1. Move should from devDependencies to dependencies.
  2. Add should to the installation instructions on README.md.
@ruimarinho
Copy link
Contributor

This is definitely a dependency, not a dev or peer one. I probably think it'd be easier to simply list the methods where they are necessary.

@ruimarinho
Copy link
Contributor

@ruiquelhas can you take of this? https://github.com/seegno/jscs-config-seegno/blame/master/src/rules/require-should-assertion-execution.js#L8 we can either cache this or mark this a dependency instead.

@ruiquelhas
Copy link
Contributor

We have the same issue for jscs, although that one is part of the installation instructions. They were peerDependencies before, to avoid this kind of issues (or at least to provide proper installation warnings).

I'm not sure if it's a good idea to move them to the dependencies, so I would suggest to just add should to the installation instructions.

@ruimarinho
Copy link
Contributor

I have a better solution for this I think - invoke the require call inside a try catch and only add this rule if the should package is installed. Since most dev envs will already have it, those who forget to install it or are simply not interested in loading it, will not suffer from it.

@fixe fixe closed this as completed in #23 Mar 31, 2016
@fixe fixe reopened this Mar 31, 2016
@fixe
Copy link
Contributor

fixe commented Mar 31, 2016

Not solved by #23. We still need to implement @ruimarinho suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants