Npm v5.10 fails to install deps for example/complex

[s1na]

I was struggling to run tests locally, because installing dependencies in example/complex failed with such an error.

One thing that seemed to solve it, was using a published version of zos-lib and not file:../.. as a dependency.

Now I noticed that, by downgrading to node v8.11 which came with npm v5.6 this problem didn't occur, and upgrading npm to v5.10 reproduced the problem. I've tried to find breaking changes in npm that might have caused this, but I haven't found one as of now.

I'm creating this issue as a reference for others facing the same issue.

Update: Even with npm v5.6, running npm i works once, but fails on retries.

Update: I've been trying to use npm ci which comes with npm v5.7.1, and it manages to install dependencies, but testing still fails with the following error:

Could not find tokens/eip20/EIP20.sol from any sources; imported from /data/home/s1na/dev/eth/zos-lib/test/mocks/mock-dependency/node_modules/truffle/test/sources/ethpm/contracts/PLCRVoting.sol

I've noticed that mock-dependency is declared as a dependency in main package and examples/complex via file:. Perhaps using lerna could fix some of the problems? and maybe greenkeeper could report these issues in advance.

[s1na]

@facuspagnuolo I was experimenting to see if I could get this to work with lerna, and actually it was easier than tracking the bug I mentioned above.

I pushed the code here: https://github.com/s1na/zos-lib/tree/monorepo Its far from polished, but with this I managed to run the tests (there were some problems, like openzeppelin-zos importing .sol files from zos-lib/contracts, which i renamed directly in node_modules for testing purposes). But I can imagine that the openzeppelin-solidity and zos-cli could also be added to the same repo as packages.

I wanted to ask your comments on this. Is this a direction you're willing to consider? Or rather should I try to somehow fix the bug with the current project structure?

[spalladino]

Hey @s1na, thanks a lot for tackling this! I am not very familiar with Lerna, but seems like a good choice for the problem. It may even worthwhile to consider merging the zos-lib and zos-cli repos into a single one if we manage it via Lerna; but this is something that we should discuss across the entire team.

I'd see if there is an easy way to fix the npm issue without introducing Lerna for now, and I'll create a separate issue to discuss if we want to go in the direction of a monorepo. Given that Travis manages to install the deps and test without any issues, I don't think we have much of a rush to tackle this. WDYT?

[s1na]

@spalladino Of course, I totally understand.

That sounds good. Here's an update:

I tried removing zos-lib as a dep in example/complex/package.json, and everywhere where it was importing the lib, I replaced it with relative path. This resolved the error in npm i. However, compiling fails due to openzeppelin-zos and the example's contracts themselves not being able to find zos-lib contracts.

Do you think maybe openzeppelin-zos and zos-lib somewhat depending on eachother could be a factor?

[spalladino]

@s1na sounds reasonable, given that openzeppelin-zos uses the Migratable contract from zos-lib. My suggestion, so you can run the examples, would be to replace the relative paths for the actual package published in npm. The only issue is that you won't be able to test any changes you make to zos-lib from the examples themselves, but we can let the CI verify everything works on the examples as soon as you send a PR.

[spalladino]

@s1na what started as a suggestion on an example, ended up with us migrating all our repos into a monorepo here. Thanks a lot for getting the ball rolling! Could you confirm if the installation now works properly for you on that repo, so we can close this issue?

[s1na]

Hey @spalladino, yes I just checked, it's working great :) thanks for being so open to a suggestion on an example!