chore(shared): replace esbuild with tsc/ts-node in libs/*#20520
chore(shared): replace esbuild with tsc/ts-node in libs/*#20520
Conversation
Because: - We want to test & develop with what we deploy. This commit: - Part of the esbuild -> tsc/ts-node migration (split of a6d1a55, PR #20390). - Pulls in libs/*, which are essentially 'shared' libs - Deletes libs/shared/nestjs/customs. This was not referenced, and created an atypical dependency direction with /libs/accounts/rate-limit.
| import { throwIntentFailedError } from './util/throwIntentFailedError'; | ||
| import type { SubscriptionAttributionParams } from './checkout.types'; | ||
| import { handleException } from 'libs/shared/error/src/lib/sanitizeExceptionsDecorator'; | ||
| import { handleException } from '@fxa/shared/error'; |
| PaymentMethodErrorType, | ||
| SubPlatPaymentMethodType, | ||
| } from '@fxa/payments/customer'; | ||
| } from '../..'; |
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| import type { ResultCart } from '@fxa/payments/cart'; |
There was a problem hiding this comment.
@StaberindeZA This was resulting in circular dep. Metrics will be at the bottom of the dependency graph. Therefore it shouldn't reference 'higher level' packages. You'll see I created a CartMetrics type below to invert this dependency chain. Please verify.
Also worth noting / kind of interesting. It turns out esbuild masked this problem; however, typescript would not build due to the circular ref, which is probably a good thing.
| @@ -28,7 +28,7 @@ import type { | |||
| StripePrice, | |||
| StripeSubscription, | |||
| } from '@fxa/payments/stripe'; | |||
| import { getPriceFromSubscription } from 'libs/payments/customer/src/lib/util/getPriceFromSubscription'; | |||
| import { getPriceFromSubscription } from '@fxa/payments/customer'; | |||
There was a problem hiding this comment.
@StaberindeZA Just trying to clean this up. Two comments though.
- Just double checkign there wasn't a reason to not to use the alias... Using the alias seems correct to me.
- As noted, metrics should be at the very bottom of the dependency graph. So I actually don't like seeing a dependency flowing in this direction. For now, it appears that
@fxa/payments/customerdoesn't depend on@fxa/payments/metrics, so this doesn't create circular ref, however, in general I don't think the metrics lib should need to reference the customer lib. This seems like it might cause some tangles down the road. Perhaps something you all want to address in a follow up.
There was a problem hiding this comment.
I agree, metrics should be at the bottom of the dependency graph.
I'm thinking maybe the best move her would be to move glean.service.ts out of @fxa/payments/metrics. I like the idea of having a class, PaymentsGleanService in this case, that you can call with minimal data that will automatically fetch whatever data it can before emitting the metric. Now it's just about where to put it.
| type StripeMetricsData, | ||
| type SubPlatCmsMetricsData, | ||
| } from './glean.types'; | ||
| import { ResultCartFactory } from '@fxa/payments/cart'; |
There was a problem hiding this comment.
@StaberindeZA Same deal, resulted in circular dependency.
Because: * The libs/payments/metrics library has the potential to cause additional ciruclar dependencies due to the modules imported from `payments-customer` by the PaymentsGleanService This commit: * Move the PaymentsGleanService to a new library, rename it and update the various dependencies. * Remove remaining circular dependencies from `payments-metrics` lib. Closes #PAY-3698
feat(payments): resolve circular dependencies
2 participants
Because
This pull request
Issue that this pull request solves
Closes: (Bunch of Issues will update soon)
Checklist
Put an
xin the boxes that applyHow to review (Optional)
Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Initially I tried to break this down top-level section, but it ended up causing issues. Combining all upgrades into single ticket.