alitnk / monopay

💳 A node.js package for making payment transactions with different payment gateways
https://monopay.js.org
MIT License
114 stars 19 forks source link

25 remove classes #33

Closed alitnk closed 2 years ago

alitnk commented 2 years ago

Closes #25

There are a lot of changes but I think the only ones that affect the users of the package are:

The features added:

Let me know if you have any ideas/concerns on the changes

alitnk commented 2 years ago

I think we could still keep requestPayment and verifyPayment, call the new request and verify functions under the hood and just mark them as deprecated.

Keivan-sf commented 2 years ago

Code is much readable now, nice job

I agree with keeping requestPayment and verifyPayment. A major change must be made softly for the users. We can delete them in the following updates later.

However driver.ts is a little complicated to read at first. I'm assuming we can have a script builder function in the utils since you've created it now to simplify the getScript a little. Also the defineDriver parameter can have a predefined type to decrease nesting a little bit.

That's it. I think clearing driver.ts clutter should do the job for us

Keivan-sf commented 2 years ago

If you're having the same thoughts I'll get on it

alitnk commented 2 years ago

Great call! Yeah you can do it if you want to.

I'll make some changes to the tests in the meantime.

Keivan-sf commented 2 years ago

I've tried to define types for the function parameters but since they're dependent to function generic types itself, they have to be defined with it like this :

type DefineDriver = <
  DriverConfigSchema extends ZodSchema,
  DriverRequestSchema extends ZodSchema,
  DriverVerifySchema extends ZodSchema,
  IConfig extends z.infer<DriverConfigSchema> & BaseConfigOptions,
  IRequest extends z.infer<DriverRequestSchema> & BaseRequestOptions,
  IVerify extends z.infer<DriverVerifySchema> & BaseVerifyOptions,
  DefaultConfig extends Partial<IConfig>,
>(param: {
  schema: { config: DriverConfigSchema; request: DriverRequestSchema; verify: DriverVerifySchema };
  defaultConfig: DefaultConfig;
  request: (arg: { ctx: IConfig; options: IRequest }) => Promise<IPaymentInfo>;
  verify: (arg: { ctx: IConfig; options: IVerify; params: Record<string, any> }) => Promise<BaseReceipt>;
}) => (config: Omit<IConfig, keyof DefaultConfig> & Partial<DefaultConfig>) => {
  request: (options: IRequest) => Promise<{
    getScript: () => string;
    referenceId: string | number;
    method: 'GET' | 'POST';
    url: string;
    params?: Record<string, any> | undefined;
  }>;
  verify: (options: IVerify, params: Record<string, any>) => Promise<BaseReceipt<any>>;
  verifyPayment: (options: IVerify, params: Record<string, any>) => Promise<BaseReceipt<any>>;
  requestPayment: (options: IRequest) => Promise<{
    getScript: () => string;
    referenceId: string | number;
    method: 'GET' | 'POST';
    url: string;
    params?: Record<string, any> | undefined;
  }>;
};

Which obviously does not make any difference in terms of nesting. I think we should leave it be

alitnk commented 2 years ago

Yeah just let TS inference do its thing - We don't really have any reason to define types for it.