Inject providers into strategies/configurable classes

[michaelbromley]

Is your feature request related to a problem? Please describe.
A number of aspects of Vendure can be customised, e.g:

• PromotionCondition
• PromotionAction
• AssetStorageStrategy
• TaxCalculationStrategy

These allow much flexibility to support various processes and business needs. However, they all lack a way to inject providers (services), which limits their usefulness.

We have ad-hoc work-arounds for this limitation, e.g. in the PromotionCondition interface we provide the check() method with a PromotionUtils object. This is not ideal though because other custom promo conditions may need other "utils" and we can't just keep inflating this object with all possible utility functions.

Describe the solution you'd like
A better solution would be to allow the injection of providers using Nest's own DI system. The recently-created JobQueueStrategy interface allows this via an init() method which receives the App's ModuleRef, which lets you inject any provider from the whole app:

vendure/packages/core/src/plugin/default-job-queue-plugin/sql-job-queue-strategy.ts

Lines 25 to 31 in 2fbf37a

	init(moduleRef: ModuleRef) {
	const processContext = moduleRef.get(ProcessContext, { strict: false });
	if (processContext.isServer) {
	this.connection = moduleRef.get(getConnectionToken() as any, { strict: false });
	this.listQueryBuilder = moduleRef.get(ListQueryBuilder, { strict: false });
	}
	}

We could therefore define a new interface, e.g:

export interface InjectableStrategy {
  init?(moduleRef: ModuleRef): void | Promise<void>;
  destroy?(): void | Promise<void>;
}

All configurable classes would implement this interface.

Describe alternatives you've considered
Another approach would be to use the Nest DI system more directly by injecting providers into the class constructor. However, this implies quite a different architecture for the way we handle the VendureConfig, since those strategies would not all need to be registered as providers in a Nest module, and it would also prevent the current pattern of configuring the strategy by instantiation.

[michaelbromley]

There are 2 distinct categories to address here:

1. Strategies: These are just interfaces which are then implemented by concrete classes.
   • AssetNamingStrategy
   • AssetPreviewStrategy
   • AssetStorageStrategy
   • TaxCalculationStrategy
   • EntityIdStrategy
   • JobQueueStrategy
   • OrderMergeStrategy
   • TaxZoneStrategy
2. ConfigurableOperationDefs: These are actual classes which are instantiated with a config object which defines the custom behaviour. They are all objects which can be configured in some way at run-time via the Admin UI:
   • PaymentMethodHandler
   • CollectionFilter
   • PromotionAction
   • PromotionCondition
   • ShippingCalculator
   • ShippingEligibilityChecker

Therefore the approach to allowing provider injection for each class of object will be different. For strategies, we simply do it as in the original proposal - implement a common interface with init and destroy methods.

ConfigurableOperationDefs (CODs) slightly trickier. Since they are already classes, we would have to add an init() and destroy() method to the config object passed to the contructor. This then presents a challenge as to where to store a reference to any injected providers. Example:

export const myPromotionAction= new PromotionOrderAction({
    code: 'my-complex-promotion',
    args: {},
    init(moduleRef) {
         // what do we do with "connection"? Hoist it to the global scope as a closure?
         // cannot assign it to "this", as we would with a Strategy.
         const connection = moduleRef.get(getConnectionToken() as any, { strict: false }); 
    },
    execute(order, args) {
        // do some calculation based on a DB lookup using "connection"
    },
    description: [/* ... */],
});

I think the general pattern above makes sense. It would be good to define a common interface which all of the config objects which are passed to the COD constructors would implement, with the init & destroy methods.

[Tyratox]

When working on a extension I noticed the issue of

what do we do with "connection"?

doesn't seem to be solved yet?

What for now seems to be working is the following (in case of a payment provider):

interface ConnectedPaymentMethodConfigOptions<
  T extends PaymentMethodArgs = PaymentMethodArgs
> extends PaymentMethodConfigOptions<T> {
  connection: Connection | null;
}
class ConnectedPaymentMethodHandler<
  T extends PaymentMethodArgs = PaymentMethodArgs
> extends PaymentMethodHandler<T> {
  constructor(config: ConnectedPaymentMethodConfigOptions<T>) {
    super(config);
  }
}
export const InvoicePaymentIntegration = new ConnectedPaymentMethodHandler({
  ....
  connection: null,
  init: function (injector) {
    this.connection = injector.getConnection();
  },
  destroy: function () {
    this.connection = null;
  },
  createPayment: async function (
    this: ConnectedPaymentMethodHandler,
    order,
    args,
    metadata
  ): Promise<CreatePaymentResult | CreatePaymentErrorResult> {
    //@ts-ignore
    const connection: Connection = this["options"].connection;

    if (!connection) {
      throw new Error(
        "This shouldn't happen"
      );
    //use connection
    }
  },
});

Now this obviously isn't pretty and a global variable might be more appropriate since only one instance is created. But it made me think of a possible solution. Using .bind() it would be possible to bind both the init() and the createPayment() functions to the instantiated class ConnectedPaymentMethodHandler where the new variables are defined as fields.

Then one could use the createPayment: (this: ConnectedPaymentMethodHandler) syntax to get proper type checking and both function would have access to the same this.

[michaelbromley]

Hi, sounds interesting. Could you illustrate the idea further with an example of the implementation so I can understand it better?

[Tyratox]

Of course!

[Tyratox]

Something like this: Tyratox@75c80b9

(Yes lifecycle.e2e-spec.ts might not be the best file for a test but as there was already an injection test in there it seemed like the easiest option for an example)

[Tyratox]

For this implementation to work it is important not to use arrow functions as these use the this from the lexically enclosing function:

An arrow function does not have its own this. The this value of the enclosing lexical scope is used; arrow functions follow the normal variable lookup rules. So while searching for this which is not present in the current scope, an arrow function ends up finding the this from its enclosing scope.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions