Fix: mark the five MikroORM database drivers as optional peer dependencies (stacked on #387) - #500
Open
AmaadMartin wants to merge 2 commits into
Open
Conversation
added 2 commits
August 1, 2026 22:00
npm >= 7 installs peerDependencies by default, so every consumer of @google/adk received @mikro-orm/mariadb, mssql, mysql, postgresql and sqlite (plus @mikro-orm/knex, knex and their native client stacks) even when they never construct a DatabaseSessionService. The drivers are already resolved through dynamic imports keyed off the connection URI scheme, so marking them optional in peerDependenciesMeta makes them opt-in without touching any runtime code.
Asserts that every MikroORM driver stays declared in both peerDependencies and peerDependenciesMeta (optional: true), that the two key sets match so a future driver cannot be added to only one, and that package-lock.json mirrors the manifest block.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Closes: #issue_number
Related: #issue_number
Problem:
core/package.jsonlists the five MikroORM SQL drivers underpeerDependenciesbut has nopeerDependenciesMetablock. As of npm v7 peer dependencies are installed by default and are only skipped when markedpeerDependenciesMeta.<name>.optional = true, so anyone who runsnpm install @google/adkalso receives@mikro-orm/mariadb,@mikro-orm/mssql,@mikro-orm/mysql,@mikro-orm/postgresql,@mikro-orm/sqlite, plus@mikro-orm/knex,knexand their transitive native client stacks — even when they never construct aDatabaseSessionService.Solution: add the missing
peerDependenciesMetaentries (five keys,{"optional": true}each) tocore/package.jsonand refreshpackage-lock.json. The runtime half of this design already landed:getConnectionOptionsFromUri(core/src/sessions/db/operations.ts) resolves each driver through a dynamicawait import()selected by the connection-URI scheme, andcore/build.jsbuilds with esbuildpackages: 'external', so nothing is imported at module load time and nothing is inlined intodist/. Only the manifest marker was missing — nocore/src/**file is touched. This mirrors the packaging postureadk-pythonalready uses for its SQL stack (an opt-indbextra rather than a base dependency).Measured effect (method below, so it can be re-run):
mainThat is a 229-package (43.7%) reduction against the base. After the change the
only
@mikro-orm/knexentries left in the tree are@mikro-orm/coreand@mikro-orm/reflection, which are real (statically imported) dependencies of@google/adk.Note: the task write-up quoted 278 → 36 packages for this measurement. Those
figures did not reproduce here; the numbers in the table are what this checkout
actually resolves, measured on both sides of the change with the command above.
Collision check (required before implementation) —
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000, thengh pr diffon every plausibly adjacent PR:feat/optional-gcp-dependencies— overlaps. It introduces thepeerDependenciesMetablock incore/package.jsonfor the three optional GCPpackages and leaves the MikroORM drivers mandatory, i.e. it inserts a block at
exactly the position this change needs to extend. Rather than build a
conflicting sibling, this PR is stacked on that branch (
--base feat/optional-gcp-dependencies) and adds its five keys to the existing block,so the combined manifest marks all eight optional peers optional.
fix/remove-phantom-mikro-orm-reflection-dep— adjacent but disjoint:it edits
dependencies(drops@mikro-orm/reflection) and addstests/integration/lazy_load_db_drivers/driver_manifest_test.ts, which pinsthe peer/dev declaration contract but does not mark anything optional. No
overlapping hunk or filename.
dev/package.json; none touchcore's peer declarations.No open PR lands this change.
Behaviour change for downstream consumers (not an API break): nothing in the
public TypeScript surface changes. A consumer who today relies on the implicit
auto-install and connects with
postgres://,mysql://,mariadb://,mssql://or — most likely in local development —sqlite://must now installthat one driver themselves; until they do,
DatabaseSessionService.init()rejects with
Cannot find module '@mikro-orm/sqlite'. That is the intendedmeaning of an optional peer dependency and matches
adk-python'sdbextra,but it is a real migration step. Nothing inside this repository is affected:
@google/adk-devtoolshard-depends on all five drivers andcorekeeps@mikro-orm/sqlitein its owndevDependencies.Deliberately out of scope: any
core/src/**change (including a friendliermissing-driver error message),
dev/package.json, removing@mikro-orm/sqlitefromcore.devDependencies, and a driver-freebuild_setupfixture — that would add a seventh networknpm installto analready install-heavy suite for a property the manifest test plus the manual
E2E below already establish.
Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
New regression test:
tests/integration/lazy_load_db_drivers/optional_peer_deps_test.ts(new file, theexisting
lazy_load_db_drivers_test.tsis untouched). It pins three things:each of the five drivers is declared in
peerDependenciesand markedoptional: true; the key sets ofpeerDependenciesandpeerDependenciesMetaare equal, so a future driver added to only one of them trips the test; and
packages.core.peerDependenciesMetainpackage-lock.jsondeep-equals themanifest block, catching a manifest edited without a lock refresh.
Proof the tests can fail (run against the unfixed manifest):
git stash push -- core/package.json, dropping the fivepeerDependenciesMetaentries: all 7 tests fail.declares @mikro-orm/mariadb as an optional peer dependency of @google/adk→AssertionError: expected undefined to be true // Object.is equality(same for mssql / mysql / postgresql / sqlite)
gives every peer dependency a peerDependenciesMeta entry→AssertionError: expected [ …(3) ] to deeply equal [ …(8) ]mirrors peerDependenciesMeta into package-lock.json→AssertionError: expected { …(8) } to deeply equal { …(3) }git stash push -- package-lock.json(simulating a manifest edit shipped without refreshing the lock): the lock
test fails alone —
AssertionError: expected { …(3) } to deeply equal { …(8) }— while the other 6 pass, showing the third assertion has independent signal.
No coverage shortfall to report: the change adds no executable source lines, so
core/srcline/branch coverage and thevitest.config.tsthresholds areunaffected.
Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
Run against a packed tarball (not a
file:link — a linked package resolvesup into the monorepo's hoisted
node_modulesand would hide the effect), from ascratch directory that has no
node_modulesin any parent:Observed:
i.e. the drivers are gone from a plain install, the missing-driver failure is
the documented opt-in prompt, and installing the single driver the URI needs
restores a working
DatabaseSessionServiceagainst a real in-memory SQLitedatabase (no mocks).
CI status: absent, validated locally instead.
.github/workflows/validation.yamland
license-check.ymlboth trigger onpull_request: branches: [main], so astacked PR based on
feat/optional-gcp-dependenciesruns no test job. On theexact commit pushed here:
(
npm run ts:checkfails on this checkout both with and without this change —2847 pre-existing errors from test files that import
@google/adk, which theroot
tsconfig.jsoncannot resolve; it is not part of the CI workflow. The newtest file contributes zero of them.)
Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.