Feat: make @google-cloud/storage and the OTel GCP exporters optional peer dependencies - #387
Open
AmaadMartin wants to merge 8 commits into
Open
Feat: make @google-cloud/storage and the OTel GCP exporters optional peer dependencies#387AmaadMartin wants to merge 8 commits into
AmaadMartin wants to merge 8 commits into
Conversation
added 8 commits
July 31, 2026 05:57
Wraps a lazy import() and converts Node's module-resolution failure into an error naming the package and the npm install command. Any other failure -- including a missing transitive dependency of the package -- is rethrown unchanged so a genuine crash is never mislabelled as "not installed".
The constructor stays synchronous and keeps its signature; the Storage client
is created on first use and memoized, so getArtifactServiceFromUri('gs://...')
never resolves @google-cloud/storage. loadArtifact and getArtifactVersion
resolve the bucket above their try block so a missing package surfaces instead
of being swallowed into undefined.
registry_test asserted the bucket name by casting into the now-removed private
'bucket' field; it now asserts the same behaviour through the public path.
getGcpExporters is already async, so each exporter is now imported only when its flag is set. Names the 5000 ms metric export interval as a constant.
…le/adk @google-cloud/storage and the two OTel Cloud exporters move out of core's runtime dependencies into optional peerDependencies (with peerDependenciesMeta, without which npm 7+ installs peers anyway) plus devDependencies so the repo's own tsc, vitest, eslint and typedoc still resolve them. @google/adk-devtools takes them on as direct dependencies, matching how it already carries all five @mikro-orm drivers, so the adk CLI keeps working unchanged.
Takes loadOptionalDependency to 100% line and branch coverage.
Replaces the two-element MODULE_NOT_FOUND_CODES Set with a direct comparison and inlines the single-use METRIC_EXPORT_INTERVAL_MS back at its call site, where exportIntervalMillis already names the value. Rewrites the error-code rationale to say when EACH code is reachable, and documents in the README that loadArtifact and getArtifactVersion now reject on a storage-client construction failure instead of returning undefined.
This was referenced Jul 31, 2026
Open
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: Every consumer of
@google/adkdownloads a large dependency closure for three packages that only two optional features use:core/src@google-cloud/storageartifacts/gcs_artifact_service.ts(GcsArtifactService)@google-cloud/opentelemetry-cloud-trace-exportertelemetry/google_cloud.ts(getGcpExporters, only whenenableTracing)@google-cloud/opentelemetry-cloud-monitoring-exportertelemetry/google_cloud.ts(getGcpExporters, only whenenableMetrics)The monitoring exporter is small itself but declares
googleapis ^137.0.0, which is by far the largest thing in the tree. Users who never touch GCS artifacts or Cloud Trace / Cloud Monitoring export pay for all of it.Solution: Move the three packages out of
core's runtimedependenciesinto optional peer dependencies, and load each with a lazyimport()at the point its feature is actually used. This follows the lazy-loading convention already established incore/src/sessions/db/operations.tsandtests/integration/lazy_load_db_drivers/rather than inventing a second pattern.Two deliberate improvements over that precedent:
core/src/utils/optional_dependency_utils.ts, turns Node's raw module-resolution failure into an actionable message:GcsArtifactService requires the optional peer dependency '@google-cloud/storage', which is not installed. Run `npm install @google-cloud/storage` to enable it.It is generically named and lives in the shared utils directory (anexpresscaller is a plausible fourth). It is internal — deliberately not added tocore/src/index.tsorcore/src/common.ts.core/package.jsongains apeerDependenciesMetablock marking only these three optional. Without it npm 7+ auto-installs peers and the change would achieve nothing. The five existing@mikro-orm/*peers are intentionally left alone — retro-fitting them would silently alter install behaviour for database users and muddy this diff.Design notes:
GcsArtifactService's constructor stays synchronous and keeps its exact signature.getArtifactServiceFromUristays synchronous. TheStorageclient is created on first use by a module-levelcreateBucket()and memoized with??=, so there is exactly one client per service instance and a rejected promise stays cached (a missing package keeps producing the same error rather than retrying resolution on every call).loadArtifactandgetArtifactVersionwrap their bodies intry { ... } catch { logger.warn(); return undefined; }.await this.getBucket()is hoisted above thetryin both, so a missing package surfaces as a real error instead of being swallowed intoundefined. Integration cases 3 and 4 pin this. See the "Two public methods can now reject" disclosure below — this is wider than just the missing-package case.getGcpExporterswas alreadyasync, so no signature changed anywhere, includingdev/src/utils/telemetry_utils.ts.dev/package.jsontakes the three on as direct dependencies, mirroring how it already carries all five@mikro-ormdrivers so the CLI supports every--session_service_urischeme out of the box. TheadkCLI's behaviour is unchanged.Bucket,File,StorageOptions,SpanProcessorandMetricReaderare nowimport typeso esbuild erases them and no runtime import survives.any,@ts-expect-error,@ts-ignore,eslint-disable,as anyoras unknown asanywhere in this diff (verified by greppinggit diff main -U0for added lines).Collision check (required before starting):
gh pr list --repo AmaadMartin/adk-js --state open --limit 100returned 100 open PRs. I diffed the plausibly-adjacent ones — #336, #295, #347, #291, #323, #303, #292, #382 — withgh pr diff <n> --name-only. None implements lazy loading or optional peer dependencies for these packages. Four touchcore/package.jsonordev/package.jsonin unrelated ways (@types/expressdeclaration,@mikro-orm/reflectionremoval, animport/no-extraneous-dependenciesrule, agoogle-auth-librarydedupe), so they overlap only as potential textual merge conflicts in different fields of the same file, not semantically. I branched frommainrather than stacking, because stacking on an arbitrary one of four peripheral overlaps would be misleading and would also suppress CI (the workflow triggers only onpull_request: branches: [main]).Two claims in the task spec turned out to be wrong; I verified against the repo and report the measured facts instead:
The spec said the ESM build emits a native
import()(ERR_MODULE_NOT_FOUND) while only the CJS build is lowered. In factcore/build.jsbuilds both node outputs withplatform: 'node'→target: ['node10.4'], so esbuild lowersawait import()to arequire()wrapper in both. Evidence, since this is easy to assume the other way round:core/dist/esm/artifacts/gcs_artifact_service.js:250andcore/dist/cjs/artifacts/gcs_artifact_service.js:265both contain() => Promise.resolve().then(() => __toESM(require("@google-cloud/storage"), 1)).core/build.js:72-78prepends acreateRequirebanner to the esm output specifically (import {createRequire as topLevelCreateRequire} from 'module'; const require = topLevelCreateRequire(import.meta.url);), which exists precisely so that loweredrequire()works in an ES module. That banner would be dead code if the esm build kept a nativeimport().Both error codes are still needed, and the helper's comment now says why each one is reachable: running from source (vitest, ts-node, or a bundler that preserves
import()) yields the native import andERR_MODULE_NOT_FOUND; the published builds yieldMODULE_NOT_FOUND.The spec said
@grpc/grpc-js"will not disappear" because@a2a-js/sdkdepends on it. It does disappear:@a2a-js/sdk@0.3.14declares@grpc/grpc-jsunderpeerDependenciesMetaas{"optional": true}, so with the exporters gone nothing pulls it in and it shows asUNMET OPTIONAL DEPENDENCYin the measured tree.One file the spec listed as "must not change" did have to change:
core/test/artifacts/registry_test.ts. Itsgs://case asserted the bucket name by casting into the private field the refactor removes —(service as unknown as {bucket: {name: string}}).bucket.name. Re-adding an equivalent private reach for the newbucketNamefield would both perpetuate the anti-pattern and add anas unknown asto the diff, so the case now mocks@google-cloud/storageand asserts the same behaviour through the public path (expect(bucketMock).toHaveBeenCalledWith('my-bucket')). ThetoBeInstanceOf(GcsArtifactService)assertion is untouched, the test name and intent are unchanged, and mutation M8 below proves the new form still catches a wrong bucket name. This is the only existing test modified by this PR — every other test change in the diff is a pure addition. (The rewrite also removes anas unknown asfrom the repo rather than adding one.)Two public methods can now reject where they previously always resolved.
GcsArtifactService.loadArtifactandgetArtifactVersionhave always wrapped their whole body intry { ... } catch (e) { logger.warn(...); return undefined; }. Becauseawait this.getBucket()is now hoisted above thattry(gcs_artifact_service.ts:114and:258), any storage-client construction failure escapes thecatch— not only the missing-package error. In practicenew Storage(options).bucket(name)does no I/O, so the realistic triggers are the missing package and an invalidStorageOptions, but the widening is real and deliberate: leaving the resolution inside thetryis exactly the bug that returns a silentundefinedto a user who forgot to install the package (mutations M3 and M6 demonstrate it). Failures of the GCS calls themselves still log and returnundefinedexactly as before. This is now stated in the README section too.Likewise
getGcpExporterscan now reject rather than always resolving. The only in-repo caller,dev/src/utils/telemetry_utils.ts:166, ships all three packages so it cannot hit this, but library consumers calling it directly withenableTracing/enableMetricscan.Behaviour change (please read): this is a behaviour change for a subset of users. Anyone who today uses
GcsArtifactService,getArtifactServiceFromUri('gs://...'), orgetGcpExporterswithenableTracing/enableMetricsand relied on the transitive install will now get the actionable error until they run onenpm install. Users of theadkCLI are unaffected. I used a non-breakingfeat:prefix rather thanfeat!:becauserelease-please-config.jsonusesrelease-type: nodewith linked versions across all four components, sofeat!would force a simultaneous major bump onmain,adk,devtoolsandintegrations— that call belongs to the maintainers, so please tell me if you would prefer this re-tagged as breaking. No migration script, codemod, or deprecation shim was added.Accepted trade-off — maintainers may want to rule on this.
dist/types/artifacts/gcs_artifact_service.d.tskeepsimport type ... from '@google-cloud/storage'becauseStorageOptionsappears in the public constructor signature. The net effect is that the dependency is optional at runtime but not at type-check time: consumers who do not install the package and compile withskipLibCheck: false(not the default) will see TS2307 on that declaration file. This is documented in the new README subsection.I deliberately did not "fix" it by widening the parameter to
unknown/anyor hand-duplicating the SDK interface. The only two fixes I can see both change the public surface, so they are your call rather than mine: (a) accept a structural options type instead ofStorageOptions, or (b) make the bucket field truly private (#bucketPromise) soBucket/StorageOptionsstay out of the emitted declaration. Happy to do either if you prefer it.One intentional deviation from the task spec: the spec asked for the
exportIntervalMillis: 5000literal to be extracted into a namedMETRIC_EXPORT_INTERVAL_MSconstant. A complexity review flagged that as single-use scope creep in a diff whose job is lazy loading, given thatexportIntervalMillis:already names the value at the call site. I agreed and left the literal inline, so the telemetry diff is now purely the lazy-import change.Scope: nothing was silently dropped. Explicitly out of scope and left alone (each is separately queued): making
expressoptional, slimming@google/adk-devtoolsso the CLI stops pullinggoogleapis, and addingpeerDependenciesMeta.optionalfor the five@mikro-orm/*drivers.docs/dependency-duplication.mdwas not created or referenced. This shipped as one PR rather than a stack because the manifest change and the lazy loading are not independently shippable — part 1 alone achieves nothing and part 2 alone breaks users; the human-authored diff is 486 insertions across 12 files, of which 238 are new tests.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.
Targeted runs (per the repo guideline against running the whole suite):
Result: 67 passed (6 files).
lazy_load_db_driversis re-run to prove the shared helper did not disturb the existing convention. All 33 pre-existinggcs_artifact_service_test.tscases and all 7 pre-existinggoogle_cloud_test.tscases pass unmodified; no test was skipped, disabled,.only'd, weakened, or deleted.New tests:
core/test/utils/optional_dependency_utils_test.ts(new, 9 cases): happy path; ESM miss; CJS miss; transitive miss rethrown by identity; module throws while evaluating; wrong error code; thrown object with a code but no message; non-Errorrejection; feature name in the message.core/test/artifacts/gcs_artifact_service_test.ts: one addeddescribeproving the constructor calls noStorageand two operations share one memoized client.core/test/telemetry/google_cloud_test.ts: one added case for both flags disabled (there was no existing both-false case).tests/integration/lazy_load_optional_gcp_deps/(new, 7 cases): all three packages mocked as unresolvable, plusgoogle-auth-librarymocked sogetGcpExportersdoes not short-circuit on a missing project id and pass vacuously on a credential-less runner.Coverage.
optional_dependency_utils.tsis at 100% statements / branches / functions / lines.google_cloud.tsis at 100% on all four.gcs_artifact_service.tsis at 97.81% / 92.75%; the only uncovered lines are 331-332 and 346-347 inside the pre-existingextractArtifactKeys/getFileNameFromPathhelpers, which this change does not touch (the highest hunk in the diff ends at line 305). No structure was made less safe to raise a number.Proof the tests can fail. Every new test was run against mutated source and confirmed to FAIL:
message.includes(`'${packageName}'`)clauseexpected Error: GcsArtifactService requires the op… to be Error: Cannot find package 'teeny-request… // Object.is equality'MODULE_NOT_FOUND'fromMODULE_NOT_FOUND_CODESexpected [Function] to throw error including 'Run `npm install @google-cloud/storag…' but got 'Cannot find module \'@google-cloud/st…'await this.getBucket()back inside thetryinloadArtifactrejects loadArtifactpromise resolved "undefined" instead of rejecting??=with a plain call ingetBucket()lazy Storage constructionexpected "spy" to be called 1 times, but got 2 timesresolves with no exporters when both flags are disabledpromise rejected "Error: [vitest] There was an error when m…" instead of resolvingawait this.getBucket()inside thetryingetArtifactVersionrejects getArtifactVersionpromise resolved "undefined" instead of rejectinglazy Storage constructionexpect(StorageMock).not.toHaveBeenCalled()new GcsArtifactService(bucket.toUpperCase())inregistry.tsregistry_testgs uricaseexpected "spy" to be called with arguments: [ 'my-bucket' ]{}fromgetGcpExporterswhen both flags are offexpected {} to deeply equal { spanProcessors: [], …(2) }Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
Install-size measurement (real consumer install, honours
peerDependenciesMeta). Run onmainand again on this branch:Measured on this machine (npm 9.2.0, Node v22.22.2):
node_modules(bytes)mainnpm ls --all --omit=devdiff — what actually left the closure (measured, not estimated):googleapis@137.1.0(111,236,216 bytes on disk here) andgoogleapis-common: gone.@google-cloud/storage(2,249,521 bytes), both Cloud exporters,@google-cloud/opentelemetry-resource-util,@google-cloud/precise-date,@google-cloud/paginator,@google-cloud/projectify,@google-cloud/promisify,teeny-request,retry-request,fast-xml-parser,form-data,yargs: gone.google-auth-library@9.15.1: 6 copies → 1. The survivor is under@google-cloud/vertexai@1.12.0. The threegoogle-auth-library@10.9.1copies are unchanged.@grpc/grpc-jsand@grpc/proto-loader: gone (2 copies each → 0). See the corrected claim above —@a2a-js/sdkdeclares them optional.UNMET OPTIONAL DEPENDENCYin the consumer tree, which is the direct evidence thatpeerDependenciesMetais doing its job.CLI unchanged, verified end to end:
prints the full command list (
web,api_server,create,run,deploy,integration) as before.CI: green.
run-testspassed on ubuntu-latest, macos-latest and windows-latest.The latest commit passed all three OS legs on the first attempt. On the first pushed commit the Windows leg needed two re-runs, both for reasons unrelated to this diff, which I am recording rather than hiding (the clean run since is further evidence they were flakes):
core/test/code_executors/unsafe_local_code_executor_test.ts > should execute shell code and return stdouttimed out at 5000ms (2692 of 2735 tests passed). That file spawns a shell and is not touched by this change —git diff main --name-onlycontains nocode_executorspath — and it is a known Windows timeout flake with dedicated fixes already in flight.Error: read ECONNRESETwhile fetching assets — a network flake.Full local validation on the same commit:
docs:checkis the one that would catch an unresolvable public-signature type, and it passes because the three packages remain installed in the repo via the newcoredevDependencies.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.