feat(payments-next): use LoggerService within TypeCachable decorators#20215
feat(payments-next): use LoggerService within TypeCachable decorators#20215david1alvarez wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the shared type-cacheable integration so cache strategy/client builder functions can use the consuming class’s LoggerService (via decorator context), and centralizes the required @type-cacheable/core global option configuration.
Changes:
- Add a shared
TypeCacheableLoggertype and thread logger instances into cache strategies/adapters. - Move
cacheManager.setOptions({ excludeContext: false })into@fxa/shared/db/type-cacheable’s barrel to enable context-aware builders across consumers. - Update multiple consuming services (Stripe, CMS, UI actions) to inject/expose a logger for use inside
@Cacheabledecorators.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/fxa-auth-server/lib/payments/stripe.ts | Passes a logger into StripeMapperService during manual construction. |
| packages/fxa-auth-server/lib/payments/initSubplat.ts | Updates StripeClient construction to pass a logger. |
| libs/shared/db/type-cacheable/src/lib/type-cachable-stale-while-revalidate-with-fallback.ts | Switches logger typing to shared TypeCacheableLogger. |
| libs/shared/db/type-cacheable/src/lib/type-cachable-cache-first.ts | Switches logger typing to shared TypeCacheableLogger. |
| libs/shared/db/type-cacheable/src/lib/type-cachable-async-local-storage-init.ts | Tightens decorator typing to require log presence on targets. |
| libs/shared/db/type-cacheable/src/lib/type-cachable-async-local-storage-adapter.ts | Adds logger support to AsyncLocalStorage adapter + factory. |
| libs/shared/db/type-cacheable/src/index.ts | Centralizes global excludeContext: false configuration. |
| libs/shared/cms/src/lib/strapi.client.ts | Removes local cacheManager option setting (now centralized). |
| libs/shared/cms/src/lib/relying-party-configuration.manager.ts | Passes the service logger into cache strategies. |
| libs/payments/ui/src/lib/nestapp/nextjs-actions.service.ts | Exposes injected logger publicly for decorator context usage. |
| libs/payments/stripe/src/lib/stripe.client.ts | Injects logger and uses context-aware strategy/client builders. |
| libs/payments/stripe/src/lib/stripe.client.spec.ts | Updates unit test DI wiring to provide a Logger. |
| libs/payments/legacy/src/lib/stripe-mapper.service.ts | Injects logger and uses context-aware strategy builder. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| this.productConfigurationManager, | ||
| { ttl: this.config.cms.legacyMapper.mapperCacheTTL } | ||
| { ttl: this.config.cms.legacyMapper.mapperCacheTTL }, | ||
| { ...this.log, log: (msg, fields) => this.log.info(msg, fields) } // StripeMapperService expects a log method, but the auth-server logger only has info, warn, and error |
There was a problem hiding this comment.
bad suggestion. Typecasting these fields to any is going to prevent typescript from catching breaking type changes.
| strategy: (_: any, context: StripeClient) => new CacheFirstStrategy(undefined, undefined, context.log), | ||
| client: (_: any, context: StripeClient) => new AsyncLocalStorageAdapter(false, context.log), |
There was a problem hiding this comment.
Not a good idea. Typecasting to any hides type changes and incompatibilities from typescripts checks, and this code is significantly less maintainable.
| @Cacheable({ | ||
| cacheKey: (args) => cacheKeyForMap(args[0], args[1]), | ||
| strategy: new CacheFirstStrategy(), | ||
| strategy: (_: any, context: StripeMapperService) => new CacheFirstStrategy(undefined, undefined, context.log), | ||
| ttlSeconds: (_, context: StripeMapperService) => |
There was a problem hiding this comment.
This is an unnecessary optimization for a legacy mapping service, and no performance hit has been seen as a result of the prior pattern.
| class TargetExposesLogger { | ||
| log!: TypeCacheableLogger; |
There was a problem hiding this comment.
See implementation in the statsd decorator https://github.com/mozilla/fxa/blob/main/libs/shared/metrics/statsd/src/lib/statsd.decorator.ts
| { | ||
| provide: Logger, | ||
| useValue: { | ||
| error: jest.fn(), | ||
| log: jest.fn(), | ||
| warn: jest.fn(), | ||
| debug: jest.fn(), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This should use our Mock* provider pattern, rather than being mocked inline (this is repeated across multiple files)
| import cacheManager from '@type-cacheable/core'; | ||
|
|
||
| // Must be disabled globally per https://github.com/joshuaslate/type-cacheable?tab=readme-ov-file#change-global-options | ||
| // otherwise @Cacheable context will be undefined when using strategy/client builder functions | ||
| cacheManager.setOptions({ | ||
| excludeContext: false, | ||
| }); |
There was a problem hiding this comment.
Can you remove the corresponding similar code within strapi.client.ts
There was a problem hiding this comment.
Already have :)
Because: * The LoggerService provides much more useful data for debugging issues, and was not being used in the TypeCachable decorator This commit: * Adds the requirement for the LoggerService to be present on the consuming classes of the TypeCachable decorators * Moves the cacheManager excludeContext call out of the strapi client and into the type-cachable barrel file, as it's now needed in several places * Updates the consuming classes to pass the LoggerService through to the decorator Closes #PAY-3571
Because:
This commit:
Closes #PAY-3571
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Any other information that is important to this pull request.