Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose MandateDetailsCreditCard type #356

Open
jp1987 opened this issue Aug 15, 2024 · 5 comments
Open

Expose MandateDetailsCreditCard type #356

jp1987 opened this issue Aug 15, 2024 · 5 comments
Assignees
Labels
enhancement Improvements and changes outside of API endpoints. need more info This needs to be discussed or investigated more.

Comments

@jp1987
Copy link

jp1987 commented Aug 15, 2024

When using TypeScript, I can't seem to import MandateDetailsCreditCard as a type through @mollie/api-client, but only like so:

import type { MandateDetailsCreditCard } from '@mollie/api-client/dist/types/src/data/customers/mandates/data';

@Pimm Pimm self-assigned this Aug 15, 2024
@janpaepke janpaepke added the enhancement Improvements and changes outside of API endpoints. label Sep 11, 2024
@janpaepke janpaepke assigned janpaepke and unassigned Pimm Sep 11, 2024
@janpaepke janpaepke added this to the 4.0.0 milestone Sep 11, 2024
@janpaepke
Copy link
Collaborator

janpaepke commented Sep 11, 2024

@Pimm: We mostly export enums from the data, but I don't think it would hurt to expose those types as well. Hence the pr.

@Pimm
Copy link
Collaborator

Pimm commented Sep 16, 2024

@janpaepke I think it could hurt, as MandateDetailsCreditCard is intended to be an internal type. Exporting it would make it part of the public API, which means we'll have to maintain it.

@jp1987 Could you elaborate on how you are currently using MandateDetailsCreditCard?

@Pimm Pimm removed this from the 4.0.0 milestone Sep 16, 2024
@janpaepke janpaepke added the need more info This needs to be discussed or investigated more. label Sep 16, 2024
@jp1987
Copy link
Author

jp1987 commented Sep 16, 2024

@jp1987 Could you elaborate on how you are currently using MandateDetailsCreditCard?

@Pimm in a custom mapper:

export const mapCreditCardMandates = (client: IClient, mandate: Mandate) => {
  const mandateDetailsCreditCard = mandate.details as MandateDetailsCreditCard;
  return {
    active: client.mollieActiveMandateId === mandate.id,
    cardHolder: mandateDetailsCreditCard.cardHolder,
    cardLabel: mandateDetailsCreditCard.cardLabel,
    cardNumber: mandateDetailsCreditCard.cardNumber,
    mandateId: mandate.id,
    status: mandate.status,
  };
};

MandataDetails is exported like this: export type MandateDetails = MandateDetailsCreditCard | MandateDetailsDirectDebit; so we need to force the type.

@Pimm
Copy link
Collaborator

Pimm commented Sep 16, 2024

Thanks for elaborating, Jesper. I think this warrants a conversation about the types we (don't) want to expose.

A discriminated union might actually help in this specific case:

if (mandate.method == 'creditcard') {
  mandate.details.cardHolder // ← guaranteed to exist inside this if-block.
}

@janpaepke
Copy link
Collaborator

janpaepke commented Sep 17, 2024

Not only would this help, but it would make the code more reliable.
For completeness, here's how I would suggest to refactor your code:

export const mapCreditCardMandates = (client: IClient, mandate: Mandate) => {
  if (mandate.method !== 'creditcard') {
    throw new Error(`Unexpected mandate method ${mandate.method}.`);
  }
  return {
    active: client.mollieActiveMandateId === mandate.id,
    cardHolder: mandateDetailsCreditCard.cardHolder,
    cardLabel: mandateDetailsCreditCard.cardLabel,
    cardNumber: mandateDetailsCreditCard.cardNumber,
    mandateId: mandate.id,
    status: mandate.status,
  };
};

This does NOT work currently, as the sdk does not expose this as a discriminated union.

@Pimm do you want to have this conversation here or open up a separate issue for this?

@janpaepke janpaepke added wontfix This will not be adressed. need more info This needs to be discussed or investigated more. and removed need more info This needs to be discussed or investigated more. wontfix This will not be adressed. labels Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements and changes outside of API endpoints. need more info This needs to be discussed or investigated more.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants