Support analytics adapters in generated Prebid bundles - #1090
ChristianPavilonis wants to merge 10 commits into
Conversation
07aa617 to
1879806
Compare
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Typed module map, versioned manifest, and analytics support for ts prebid bundle. The resolver hardening is the strongest part of this change and I have no correctness objections to the implementation. Everything below is non-blocking: one documentation gap on the operator upgrade path, one test assertion that is weaker than the PR description claims, two maintainability items, and one follow-up that is out of scope for this PR.
3 of the inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch) to apply them. Each was applied to a scratch worktree at this head and verified before posting:npx prettier --check,npx vitest run test/prebid-artifact-integration.test.mjs(5/5), anddocsprettier via the pinneddocs/node_modulesbinary. The three together produce no drift beyond the approved ranges. The remaining comments are prose because the fix spans multiple hunks or files.
Non-blocking
🤔 thinking
- Operator upgrade path is not written down anywhere but a CLI error string — see inline at
docs/guide/integrations/prebid.md:185
♻️ refactor
expectNoUnexpectedNetworkActivitynever asserts onstubs.requests— see inline atcrates/trusted-server-js/lib/test/prebid-artifact-integration.test.mjs:198- Metadata directory is re-read once per selected module — see Cross-cutting below
- Pinned Prebid version hardcoded in three assertions — see Cross-cutting below
⛏ nitpick
- Test name describes one artifact, body asserts both — see inline at
crates/trusted-server-js/lib/test/prebid-artifact-integration.test.mjs:417
📌 out of scope
- Analytics adapters compile into a bundle with no TCF/GPP purpose enforcement — see Cross-cutting below
👍 praise
- Resolver hardening — see Cross-cutting below
Cross-cutting / body-level findings
-
♻️ Metadata directory is re-read once per selected module —
resolveOneModulecallsfs.readdirSync(context.metadataDir)inside the per-module loop (crates/trusted-server-js/lib/build-prebid-external.mjs:307), and that directory holds 665 entries in the pinned package. The readdir itself is the right call — it is an exact-case membership test, whichfs.existsSyncwould not give you on a case-insensitive filesystem, and theATSanAlyticsAdaptertest depends on that behaviour — it just does not need repeating per module. Hoist it intoresolveBundleModulesbeside thecanonicalMetadataDirvalidation and thread aSetthroughcontext:// resolveBundleModules, after canonicalMetadataDir is validated: const metadataEntries = new Set(fs.readdirSync(canonicalMetadataDir)); // ...include metadataEntries in the context object passed to resolveOneModule // resolveOneModule: if (!context.metadataEntries.has(metadataFilename)) { throw unsupportedModuleError(definition, stem, context.prebidVersion); }
Apply manually — this touches two non-adjacent hunks, so it cannot be a single
suggestion. -
♻️ Pinned Prebid version hardcoded in three assertions that are not about the version —
crates/trusted-server-js/lib/test/build-prebid-external.test.mjs:52,:671,:739, andcrates/trusted-server-js/lib/test/prebid-artifact-integration.test.mjs:279all pin10.26.0. A prebid.js bump breaks four assertions whose subject is module resolution, not the version number.verifyPrebidPackageVersionis already exported and already imported in the first file — deriving it once at module scope (const prebidVersion = verifyPrebidPackageVersion();) and substituting keeps the version-interpolation assertion at:671meaningful while removing the coupling. ThewriteVersionFixturesliterals are correct as they are; those are the subject of their own tests.Apply manually — spans two files.
-
📌 Analytics adapters now compile into a bundle with no TCF/GPP purpose enforcement —
renderExternalEntryimportsconsentManagementTcf,consentManagementGpp, andconsentManagementUsp(crates/trusted-server-js/lib/build-prebid-external.mjs:526-528) but nottcfControlorgppControl. TheconsentManagement*modules read and expose the consent string; the*Controlmodules are what actually gate vendor activity on denied purposes. That gap predates this PR, but this PR is what makes it straightforward to compile an identity-analytics vendor into the bundle. Publisher JavaScript ownsenableAnalytics, so a publisher can gate it themselves — but the operator has no way to add the enforcement modules throughbundle.modules, since they are not bidder, userId, or analytics component types. I could not find an open issue tracking this. Worth filing one before analytics ships to a GDPR publisher; no change requested here. -
👍 Resolver hardening —
realpathcontainment checks on both the metadata file and the resolved package export, with a symlink-escape test for each; exact-case metadata matching; the lockfile-versus-installed version check with actionablenpm ciguidance; and the removal of the old silent fallback that stamped the module stem as a bidder code when metadata was missing. AssertingcreateGeneratedPathsandbuildBundlewerenot.toHaveBeenCalled()on every failure path is a precise way to pin the "fail before Vite" ordering, and it is done consistently across six tests.
CI Status
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- browser integration tests: PASS
- CodeQL: PASS
- Analyze (javascript-typescript): PASS
- cargo test (ts CLI, native): PASS
- format-docs: FAIL (required)
- format-typescript: PASS (required)
- Analyze (rust): PASS
- cargo test (axum native): PASS
- cargo test: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- prepare integration artifacts: PASS
- vitest: PASS
- Analyze (actions): PASS
- cargo fmt: PASS (required)
format-docs is two whitespace-only deviations in docs/guide/integrations/prebid.md, both introduced by 1e52d790: a missing blank line after the ### Browser configuration options heading (line 79), and a doubled blank line before ## External Bundle Generation (line 144). Reviewer confirmed those are the only two, and that fixing them makes cd docs && npm run format pass. They were deliberately excluded from this review's findings at the reviewer's request, but format-docs is a required check, so merge stays blocked until they are fixed.
aram356
left a comment
There was a problem hiding this comment.
Summary
Typed module map, upstream-only resolution, and the schema-versioned manifest are coherent across the CLI, generator, and shim, and the test coverage is thorough. Two blocking problems: the core runtime config still models the old bundle.adapters shape, so the config this PR documents cannot be validated, pushed, or loaded; and the required format-docs check fails on docs/guide/integrations/prebid.md.
4 of the inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. The remaining comments describe the fix in prose because the change touches other files or lines outside the diff and can't be auto-applied.
Blocking
🔧 wrench
- Core runtime config still rejects
bundle.modules— see inline atcrates/trusted-server-cli/src/prebid_bundle.rs:67 format-docs(required) fails: missing blank line after heading — see inline atdocs/guide/integrations/prebid.md:79format-docs(required) fails: extra blank line before heading — see inline atdocs/guide/integrations/prebid.md:144
Non-blocking
⛏ nitpick
- Version-specific analytics claim will rot — see inline at
docs/guide/integrations/prebid.md:564 - Unrelated override-rules example added to the config template — see inline at
trusted-server.example.toml:470 - Metadata directory re-read per selected stem — see inline at
crates/trusted-server-js/lib/build-prebid-external.mjs:307
Cross-cutting / body-level findings
- 🌱 Shim parses
runtimeCodes.analyticsbut never uses it —getExternalBundleManifest()validates and types the analytics runtime codes, then nothing reads them. The bidder codes drive theclient_side_bidderscheck; the analytics codes could drive an equivalent one. A follow-up could wrappbjs.enableAnalyticsand log an operator-facing error naming[integrations.prebid.bundle.modules].analyticswhen the requested provider is not in the manifest, since Prebid's ownno analytics adapter found in registrymessage does not mention the config key that fixes it.
CI Status
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- browser integration tests: PASS
- CodeQL: PASS
- Analyze (javascript-typescript): PASS
- cargo test (ts CLI, native): PASS
- format-docs: FAIL (required)
- format-typescript: PASS (required)
- Analyze (rust): PASS
- cargo test (axum native): PASS
- cargo test: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- prepare integration artifacts: PASS
- vitest: PASS
- Analyze (actions): PASS
- cargo fmt: PASS (required)
The github-code-quality bot's fakeBundleMetadata comment is stale; that helper no longer exists at this head.
Summary
pbjs.enableAnalyticscan use adapters such as ATS without missing-registry errors.Changes
crates/trusted-server-cli/src/prebid_bundle.rscrates/trusted-server-js/lib/build-prebid-external.mjscrates/trusted-server-js/lib/src/integrations/prebid/index.tscrates/trusted-server-js/lib/src/integrations/prebid/user_id_modules.jsoncrates/trusted-server-js/lib/src/integrations/prebid/user_id_modules.tsimportPathfield from the registry type.crates/trusted-server-js/lib/test/build-prebid-external.test.mjscrates/trusted-server-js/lib/test/integrations/prebid/index.test.tscrates/trusted-server-js/lib/test/prebid-artifact-integration.test.mjsdocs/guide/cli.mddocs/guide/integrations/prebid.mddocs/superpowers/plans/2026-08-28-prebid-bundle-module-map.mddocs/superpowers/specs/2026-08-28-prebid-bundle-module-map-design.mdtrusted-server.example.tomlScope
This pull request changes the Rust command-line interface, Node.js bundle generator, browser diagnostics, tests, and operator documentation because they share one configuration and manifest contract. It is limited to modules shipped by the locked Prebid.js package. It does not add custom module paths, download third-party adapters, or move
pbjs.enableAnalyticsoptions out of publisher code.Closes
Closes #1085
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo test-cloudflare,cargo test-spin, all six target-matched Clippy commands, parity tests, CLI tests, JavaScript build, documentation build, a real bundle smoke test, and the unsupported-module failure path.Checklist
unwrap()in production code; new fallible paths return actionable errors