Making chai a dependency

[startgeek]

hey everyone what's up
it's kind of surprising for me too since truffle should come with chai natively but I guess it only has it in form of internal use and doesn't provide it to external modules such as openzeppelin-test-helper
missing chai dependency and when I npm install it manually it fixes here's a snapshot

[frangio]

Thanks for reporting @startgeek! Funny, @nventuro and I were talking about this exact thing earlier today.

Actually chai is a peer dependency of this package. This means that you have to install both side by side. The reason we made it that way is that we wanted to make sure to be using exactly the same version of chai that the user of this package is using. However... I don't think this is a problem anymore after removing chai "autoinstall" in #18. We might be able to make it an actual dependency now, and the user experience will be a lot better. @nventuro What do you think?

[nventuro]

Hm, yes, I'm not sure why I made it a peer dependency on that peer, having a regular dependency makes sense. We should fix this for v0.4.

[startgeek]

I believe one step that could be taken before each commit is that I (as a developer ) make a clean fresh environment (probably on VMWARE) with truffle wired and a bunch of heavy contracts and test and before each commit, I could do:

1. truffle init
   2.copy contracts and test files
   3.npm install openzeppelin-solidity
   4.npm install openzeppelin-test-helpers
   5.truffle test
   and it should pass all of the test's
   this way we could be sure that it doesn't access chai from a global npm repo(which I installed globally once by accident)

[startgeek]

oh and I have this question how I can use openzeppelin-test-helpers in migration files?
is it a good idea?
we should consider making another library just for this purpose

[frangio]

@startgeek That's a bit unrelated to this issue, it sounds more like the kind of thing to discuss in the forum. 🙂 https://forum.zeppelin.solutions

[frangio]

Hm, actually we still need chai as a peer dependency for this to work:

openzeppelin-test-helpers/src/setup.js

Lines 1 to 4 in 8a2de42

	const chai = require('chai');
	const BN = web3.utils.BN;
	chai.use(require('chai-bn')(BN));

I'm willing to let go of this last bit of magic setup. I think it would be much better if we were completely agnostic to chai. 🙂

---

An ugly hack alternative to have chai as a dependency and have the above still work is to use module.parent.require('chai') to install chai-bn on the parent module.

[nventuro]

Wouldn't chai-bn also become a peer-dependency in that case too?

@startgeek

I believe one step that could be taken before each commit is that I (as a developer ) make a clean fresh environment (probably on VMWARE) with truffle wired and a bunch of heavy contracts and test and before each commit, I could do:
truffle init
2.copy contracts and test files
3.npm install openzeppelin-solidity
4.npm install openzeppelin-test-helpers
5.truffle test
and it should pass all of the test's
this way we could be sure that it doesn't access chai from a global npm repo(which I installed globally once by accident)

We actually already do this :) I've set up a couple integration tests, and as you can see, they have chai as an explicit dependency.

[frangio]

Wouldn't chai-bn also become a peer-dependency in that case too?

No, we can use it internally without forcing users of test-helpers to use it too.

[nventuro]

@frangio we should make a decision about this for the v0.4 release.

[frangio]

Do we want to remove auto installation of chai-bn?

[nventuro]

I'd rather keep boilerplate to a minimum, and chai-bn is IMO too magical for a non-js expert (which is often the case nowadays in the industry): I think it'd be best if we simply hide it from our users. The whole purpose of this package is to make testing simpler, after all.

[frangio]

Then we can make chai a dependency of test-helpers in order to use it internally (do we even need it anyway?) and additionally if chai is installed next to test-helpers, we can register the chai-bn plugin into it.

What is magical about chai-bn though? It's a chai plugin... We can show people how to use it.

[jamesmorgan]

I just stumbled upon this when using the latest test helpers. Seem strange to have to install both chai and chai-bn separately, especially since no docs I could find point this out (at least no obvious ones)

[frangio]

@jamesmorgan While I agree that it's annoying to have to install chai, it is specified as a peer dependency:

openzeppelin-test-helpers/package.json

Lines 57 to 59 in 4fa2d9e

	"peerDependencies": {
	"chai": "^4.2.0"
	}

This should be enough but npm isn't loud enough about these. We are missing documenting this more clearly, but I'd much rather we removed the peer dependency entirely.

chai-bn I don't think you should be installing manually, it's a dependency of openzeppelin-test-helpers.

[nventuro]

The docs now indicate that chai should be installed, and we haven't received any reports related to this recently. Closing.

[frangio]

It's still bad UX that users have to install chai manually, due to test-helpers using it internally.

[VanijaDev]

Hi, strange but I still get Error: Cannot find module 'chai'

[frangio]

Thank you for reporting @VanijaDev! No, it's not strange. I implemented it wrong and didn't test it correctly. 🤦‍♂️

I'm preparing a fix and will publish it in a short while.

[frangio]

@VanijaDev v0.5.3 has been released with the fix.