Orbit-db: make clean leaves dirty package.json files

[RichardLitt]

We should change make clean to not do this:

11:20 ~/src/orbitdb/orbit-db (master) * 🐕  make clean
rm -rf orbitdb/
rm -rf node_modules/
rm package-lock.json
11:20 ~/src/orbitdb/orbit-db (master) * 🐕  git k
* master
RichardLitt [email protected]:RichardLitt/orbit-db.git (fetch)
RichardLitt [email protected]:RichardLitt/orbit-db.git (push)
origin  [email protected]:orbitdb/orbit-db.git (fetch)
origin  [email protected]:orbitdb/orbit-db.git (push)
On branch master
Your branch is ahead of 'origin/master' by 38 commits.
  (use "git push" to publish your local commits)
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

    deleted:    package-lock.json
    modified:   package.json

no changes added to commit (use "git add" and/or "git commit -a")

[thiagodelgado111]

Tried to reproduce that from master. It did remove package-lock.json but it didn't modify package.json here. BTW, should make clean remove that lockfile?

[RichardLitt]

Did you run npm install first?

package-lock.json should be checked in to source control, yeah. It shouldn't be removed.

[thiagodelgado111]

Yep. You didn't install anything, just installed dependencies, right?

[RichardLitt]

Yeah. Just tried again; the only thing make clean did that it shouldn't was delete the package-lock.json file, which it oughtn't to do.

[thiagodelgado111]

I think it does the trick: https://github.com/orbitdb/orbit-db/pull/401/files ?

[haadcode]

The intention for removing package-lock.json in clean was to provide a way to rebuild everything from scratch since package-lock can contain binary modules that are tied to eg. OSX. By running make clean && make devs on Linux could get everything installed as their platform requirements.

This is very valuable functionality in the dev workflow. If we want to change clean to not remove package-lock, let's make sure there's a command that replaces it. I propose moving the removal of package-lock to make rebuild command (and in rebuild, run clean as well) as then:

• make clean - remove all created files from the repo (ie. reset directory state)
• make rebuild - build everything from scratch

What do you think?

[RichardLitt]

That makes a lot of sense to me! Thanks for explaining that. I think we should split it out into the two commands; at the moment, make clean implies that it should clean the slate, for me, and not that it should remove package-lock.json. If there are different use cases for the different commands, I don't see a major downside for adding rebuild. Let's do it.

[thiagodelgado111]

Just to make sure, this is the problem with the package-lock.json you're referring to?

• https://github.com/npm/npm/issues/17722
• https://github.com/npm/npm/issues/18202
• https://github.com/npm/npm/issues/18135

Honest question: If a dev using a different platform have to remove the lockfile to install everything from scratch using npm install, doesn't it defeats the purpose of having a lockfile?

Never tried using yarn with orbit-db but it seems that that problem doesn't occur with yarn

[haadcode]

Iirc yarn has a different/its own lockfile. I don't feel it defeats the purpose, which is to make sure that the deps are locked to certain versions thus avoiding the problem of pulling in broken deps (this has happened several times in the past when a patch version update broke something).

It's unfortunate that native bins for different platforms are not handled that well in npm, but I'm sure they'll find a solution for it eventually.

[RichardLitt]

Yarn does have it's own lockfile. I think that the multi-command solution works the best, I think.

[thiagodelgado111]

Got it! Just for the records, I didn't mean to criticize npm :)

which is to make sure that the deps are locked to certain versions thus avoiding the problem of pulling in broken deps

I had the feeling that if the lockfile is removed, installing things from scratch could cause that. I guess it's just a matter of doing a thorough review and diffing to make sure the package-lock changes aren't committed and packages aren't updated when they shouldn't, etc :)

[shamb0t]

Believe this was fixed in #499, feel free to reopen if not :)