MasterKale / SimpleWebAuthn

WebAuthn, Simplified. A collection of TypeScript-first libraries for simpler WebAuthn integration. Supports modern browsers, Node, Deno, and more.
https://simplewebauthn.dev
MIT License
1.62k stars 137 forks source link

Stop hoisting deps #276

Closed MasterKale closed 2 years ago

MasterKale commented 2 years ago

A couple of things lately have confused me about this monorepo's Lerna setup:

  1. Why weren't package-lock.json files within packages/ ignored? (https://github.com/MasterKale/SimpleWebAuthn/pull/267#pullrequestreview-1114650433)
  2. Why has Lerna been eager to start including "devDependencies" in package.json files within packages/ as "dependencies" in the root package-lock.json? (https://github.com/MasterKale/SimpleWebAuthn/issues/273#issuecomment-1260579047)

Something seems to have changed in Lerna since I initially added the --hoist option back in #213, or I've misunderstood this whole time. In any case this PR should help synchronize how I want monorepo dependencies to be managed with how they are actually managed by Lerna.

MasterKale commented 2 years ago

Hmm, npm ci isn't happy with my trying not to hoist anymore:

> bootstrap
> lerna bootstrap

lerna notice cli v5.4.3
lerna info ci enabled
lerna info Bootstrapping 4 packages
lerna info Installing external dependencies
lerna ERR! npm ci exited 1 in '@simplewebauthn/server'
lerna ERR! npm ci stderr:
npm ERR! code EUSAGE
npm ERR! 
npm ERR! The `npm ci` command can only install with an existing package-lock.json or
npm ERR! npm-shrinkwrap.json with lockfileVersion >= 1. Run an install with npm@5 or
npm ERR! later to generate a package-lock.json file, then try again.
npm ERR! 
npm ERR! Clean install a project
npm ERR! 
npm ERR! Usage:
npm ERR! npm ci
npm ERR! 
npm ERR! Options:
npm ERR! [--no-audit] [--foreground-scripts] [--ignore-scripts]
npm ERR! [--script-shell <script-shell>]
npm ERR! 
npm ERR! aliases: clean-install, ic, install-clean, isntall-clean
npm ERR! 
npm ERR! Run "npm help ci" for more info

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/runner/.npm/_logs/2022-09-28T16_06_09_258Z-debug-0.log
lerna ERR! npm ci exited 1 in '@simplewebauthn/server'
lerna WARN complete Waiting for 1 child process to exit. CTRL-C to exit immediately.

It seems with hoisting I can get away with no package-lock.json files in the individual packages, since npm ci will use the root package-lock.json instead.

I might not need this PR after all, or if I keep it it's to fix the definitely incorrect gitignore entry to the other package-lock.json files 🤔

MasterKale commented 2 years ago

I found this interesting issue on the Lerna repo from a few years ago, a comment in there made me wonder if I shouldn't move all package devDependencies into the root package.json:

https://github.com/lerna/lerna/issues/1256#issuecomment-365814491

The thread in general also details how Lerna updated package-lock.json on an initial git clone -> npm install, which might explain why others contributing to the project are including root package-lock.json changes that aren't really necessary in my mind. I'm not happy with this right now because it means that "dependencies" in browser and server are being versioned at the root level, but not within the individual packages. I don't want dependencies like this versioned at all...I need to investigate more.

MasterKale commented 2 years ago

Alright, I think I got things where I want them. Now, the following flow:

  1. Run git clone to pull down this repo
  2. Run npm install to install root dependencies
  3. Run npm run bootstrap to build the packages

...Will have the following desired outcomes:

  1. Won't create local changes to the root package-lock.json
  2. Will continue to allow "dependencies" and "devDependencis" to be defined per-package
    • To date I've only ever defined package-specific dependencies in the various package.json's
  3. Won't make changes to the root package-lock.json when I lerna add a new dependency to a package
  4. Will still support CI execution, which was the original reason for needing to hoist