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.

Sign up for GitHub

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

Purchases: Convert me/purchases/utils to TS #97297

Merged
merged 4 commits into from
Dec 12, 2024
Merged

Purchases: Convert me/purchases/utils to TS #97297

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2a9d764
Convert me/purchases/utils to TS
sirbrillig Dec 10, 2024
b58596b
Prevent calling purchase functions without purchase
sirbrillig Dec 10, 2024
9e5efd8
Export utils functions directly
sirbrillig Dec 10, 2024
9f85309
Add explicit return types to all util functions
sirbrillig Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions client/me/purchases/manage-purchase/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,9 @@ class ManagePurchase extends Component<
const { purchase, siteSlug, redirectTo } = this.props;
const options = redirectTo ? { redirectTo } : undefined;
const isSitelessRenewal =
isAkismetTemporarySitePurchase( purchase ) || isMarketplaceTemporarySitePurchase( purchase );
purchase &&
( isAkismetTemporarySitePurchase( purchase ) ||
isMarketplaceTemporarySitePurchase( purchase ) );

if ( ! purchase ) {
return;
Expand Down Expand Up @@ -1692,10 +1694,9 @@ export default connect( ( state: IAppState, props: ManagePurchaseProps ) => {
hasSetupAds: Boolean(
site?.options?.wordads || isRequestingWordAdsApprovalForSite( state, site )
),
hasCompletedCancelPurchaseSurvey: getPreference(
state,
getCancelPurchaseSurveyCompletedPreferenceKey( purchase?.id )
),
hasCompletedCancelPurchaseSurvey: purchase
? getPreference( state, getCancelPurchaseSurveyCompletedPreferenceKey( purchase.id ) )
: false,
isAtomicSite: isSiteAtomic( state, siteId ),
isDomainOnlySite: purchase && isDomainOnly( state, purchase.siteId ),
isProductOwner,
Expand Down
45 changes: 16 additions & 29 deletions client/me/purchases/utils.js → client/me/purchases/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,16 @@ import {
isPaidWithCreditCard,
} from 'calypso/lib/purchases';
import { addPaymentMethod, changePaymentMethod, addNewPaymentMethod } from './paths';
import type { Purchase } from 'calypso/lib/purchases/types';

function isDataLoading( props ) {
export function isDataLoading( props: {
hasLoadedSites: boolean;
hasLoadedUserPurchasesFromServer: boolean;
} ): boolean {
return ! props.hasLoadedSites || ! props.hasLoadedUserPurchasesFromServer;
}

function canEditPaymentDetails( purchase ) {
export function canEditPaymentDetails( purchase: Purchase ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this would expect a boolean as a response. Should it be included as the return type here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good question! It does have an implicit boolean return value, but TypeScript can infer this and got lazy 😅 . Unlike PHP, inferred types are just as valid and strict as explicit types, so if if you tried to, say, use string manipulation on the result of this function, you wouldn't be able to compile it (eg: "cannot use this boolean as a string").

The advantage of adding an explicit return type would be that if this function currently or in the future tried to return something different, a type error would appear here (eg: "this function returns a boolean but it can return a string"). By leaving the return type off, we're telling TypeScript, "the return type of this function is whatever it can return". If we modified it to sometimes return a string, the implicit return type would become boolean|string and TypeScript would make sure that everywhere that called it was ok with having a string sometimes.

It's a trade-off either way. This SO answer does a pretty good job of explaining it. I generally prefer adding return types but when converting existing code sometimes I don't as an expedient. That said, I may go back and add them here.

Copy link
Member Author

@sirbrillig sirbrillig Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example of the inferred types at work. When converting this function to TS maybe I don't know what it's supposed to return, so I can just let TS figure it out:

Screenshot 2024-12-12 at 3 39 37 PM

That's usually the reason to leave off a return type: it's not clear or maybe it's something really complicated, in which case TS can handle it.

return (
! isExpired( purchase ) &&
! isOneTimePurchase( purchase ) &&
Expand All @@ -22,63 +26,46 @@ function canEditPaymentDetails( purchase ) {
);
}

function getChangePaymentMethodPath( siteSlug, purchase ) {
if ( isPaidWithCreditCard( purchase ) ) {
const {
payment: { creditCard },
} = purchase;

return changePaymentMethod( siteSlug, purchase.id, creditCard.id );
export function getChangePaymentMethodPath( siteSlug: string, purchase: Purchase ) {
if ( isPaidWithCreditCard( purchase ) && purchase.payment.creditCard ) {
return changePaymentMethod( siteSlug, purchase.id, purchase.payment.creditCard.id );
}

return addPaymentMethod( siteSlug, purchase.id );
}

function getAddNewPaymentMethodPath() {
export function getAddNewPaymentMethodPath() {
return addNewPaymentMethod;
}

function isTemporarySitePurchase( purchase ) {
export function isTemporarySitePurchase( purchase: Purchase ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be returning a string? If so, should be typed as such? When is it best to type the return and when not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully I was able to explain this somewhat above. TLDR when converting existing functions, IMO it's ok to leave off return types. It's way more important to have a file be using TypeScript than for it to have perfect types.

const { domain } = purchase;
// Currently only Jeypack, Akismet and some Marketplace products allow siteless/userless(license-based) purchases which require a temporary
// site(s) to work. This function may need to be updated in the future as additional products types
// incorporate siteless/userless(licensebased) product based purchases..
return /^siteless.(jetpack|akismet|marketplace.wp).com$/.test( domain );
}

function getTemporarySiteType( purchase ) {
export function getTemporarySiteType( purchase: Purchase ) {
const { productType } = purchase;
return isTemporarySitePurchase( purchase ) ? productType : null;
}

function isAkismetTemporarySitePurchase( purchase ) {
export function isAkismetTemporarySitePurchase( purchase: Purchase ) {
const { productType } = purchase;
return isTemporarySitePurchase( purchase ) && productType === 'akismet';
}

function isMarketplaceTemporarySitePurchase( purchase ) {
export function isMarketplaceTemporarySitePurchase( purchase: Purchase ) {
const { productType } = purchase;
return isTemporarySitePurchase( purchase ) && productType === 'saas_plugin';
}

function isJetpackTemporarySitePurchase( purchase ) {
export function isJetpackTemporarySitePurchase( purchase: Purchase ) {
const { productType } = purchase;
return isTemporarySitePurchase( purchase ) && productType === 'jetpack';
}

function getCancelPurchaseSurveyCompletedPreferenceKey( purchaseId ) {
export function getCancelPurchaseSurveyCompletedPreferenceKey( purchaseId: string | number ) {
return `cancel-purchase-survey-completed-${ purchaseId }`;
}

export {
canEditPaymentDetails,
getChangePaymentMethodPath,
getAddNewPaymentMethodPath,
isDataLoading,
isTemporarySitePurchase,
getCancelPurchaseSurveyCompletedPreferenceKey,
getTemporarySiteType,
isJetpackTemporarySitePurchase,
isAkismetTemporarySitePurchase,
isMarketplaceTemporarySitePurchase,
};
Loading