Skip to content

chore: Minimal yarn installs#403

Merged
michaelkirk merged 4 commits intoheadwaymaps:mainfrom
3nprob:safer-yarn-install
Dec 11, 2025
Merged

chore: Minimal yarn installs#403
michaelkirk merged 4 commits intoheadwaymaps:mainfrom
3nprob:safer-yarn-install

Conversation

@3nprob
Copy link
Contributor

@3nprob 3nprob commented Dec 8, 2025

Tweak the installation procedure for npm packages:

  • Run yarn install with --ignore-scripts to avoid running unnecessary lifecycle scripts
    • Reduces supply-chain risk of compromised dependencies
    • tileserver: Add explicit execution of required sqlite3 dependency install script
  • Run yarn install with --frozen-lockfile where not already doing so
  • Trim devDependencies from runtime images
    • Reduce image sizes by a few 100MBs total
    • frontend: Move @quasar/cli from devDependencies to dependencies

@3nprob 3nprob changed the title Safer yarn install Minimal yarn installs Dec 8, 2025
@3nprob 3nprob marked this pull request as ready for review December 8, 2025 22:59
@3nprob 3nprob changed the title Minimal yarn installs chore: Minimal yarn installs Dec 8, 2025
@michaelkirk
Copy link
Member

tileserver: Add explicit execution of required sqlite3 dependency install script

Was this a consequence of adding --ignore-scripts? My guess is you dont want to trust scripts to run, but we need this to happen, so you effectively copied the one script we actually need to run? Is that right?

frontend: Move @quasar/cli from devDependencies to dependencies

What was the motivation for this change?

@3nprob 3nprob force-pushed the safer-yarn-install branch from ec932aa to ad48b66 Compare December 10, 2025 23:13
@3nprob
Copy link
Contributor Author

3nprob commented Dec 10, 2025

tileserver: Add explicit execution of required sqlite3 dependency install script

Was this a consequence of adding --ignore-scripts? My guess is you dont want to trust scripts to run, but we need this to happen, so you effectively copied the one script we actually need to run? Is that right?

That's right! Since it's just the one it seems reasonable to just run it explicitly. If there'd be more, there are more engineered options like pnpm's onlyBuiltDependencies.

frontend: Move @quasar/cli from devDependencies to dependencies

What was the motivation for this change?

I guess I did that to keep supporting running bin/dev-server from the image, which I now think isn't really necessary. I just dropped this change.

@michaelkirk
Copy link
Member

That's right! Since it's just the one it seems reasonable to just run it explicitly. If there'd be more, there are more engineered options like pnpm's onlyBuiltDependencies.

I'm sympathetic to the concern, but I have some questions. Note also that some rust packages have a similar build script.

So what happens when we do a yarn install for a package that has a necessary build script? Is there obvious error output at runtime like "build script is required for the sqlite3 package" or is it an inexplicable error at install time? Or a silent failure that presents at runtime?

(everything else in the PR seems uncontroversial to me)

@3nprob
Copy link
Contributor Author

3nprob commented Dec 11, 2025

So what happens when we do a yarn install for a package that has a necessary build script? Is there obvious error output at runtime like "build script is required for the sqlite3 package" or is it an inexplicable error at install time? Or a silent failure that presents at runtime?

With vanilla yarn1, there will be a warning about script being skipped at install time (already present for this PR), and then crashing hard due to the missing library on import/require at runtime, covered in CI (I don't have the exact error handy but IIRC it's obvious enough - the exact runtime error and how obvious it is may vary between dependencies).

@michaelkirk
Copy link
Member

Alright, let's give it a shot! Thanks for the explanation and shrinking down the deployed containers.

@michaelkirk michaelkirk added this pull request to the merge queue Dec 11, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 11, 2025
@michaelkirk michaelkirk added this pull request to the merge queue Dec 11, 2025
Merged via the queue into headwaymaps:main with commit b2aaa97 Dec 11, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants