Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Update @woo…/currency code docs, define & export type declarations. #7848

Closed
wants to merge 5 commits into from

Conversation

tomalec
Copy link
Member

@tomalec tomalec commented Oct 26, 2021

⚠️ This PR is based on/requires #7840
And served the same purpose:

As per the Dependency Extraction Webpack Plugin installing this package in another plugin serves only type checking purposes (see WordPress/gutenberg#35630 (comment)), it would be beneficial to actually define and export those types.

  • Removes unrelated Google Search link form README (1c4c8b4)
  • Defines CurrencyConfig type, exports declarations (afa01e3)
    So they could be referenced in the other packages.

Accessibility

n/a - its only code documentation change

Screenshots

image

Detailed test instructions:

  • npm install @woocommerce/currency
  • Somewhere in your codebase's JSDoc use {import('@woocommerce/currency').CurrencyConfig}

Additional notes:

  1. This data type could be used further in @woocommerce/wc-admin-settings to make the code docs shorter.

  2. Preferable I'd not define CurrencyProps type, just do

    /**
     * @typedef {NumberConfig} CurrencyConfig
     * @property {string} code Currency ISO code.
     * 

    or at least

    /**
     * @typedef {NumberConfig & {code: string}} CurrencyConfig
     * @property {string} code Currency ISO code.
     * 

    But unfortunately, it's not supported by TypeScript yet Support typedef inheritance with JSDoc microsoft/TypeScript#20077

  3. The type of the object returned by CurrencyFactory is still not defined, and the individual methods of that object are lost as well. I'd solve that in a separate PR, as I believe we could do a small refactoring by the way. The current code base makes it hard to document and test it. At least that makes this PR shorter and less controversial.


No changelog necessary - added entry in package's one.

* @typedef {import('@woocommerce/number').NumberConfig} NumberConfig
*/
/**
* @typedef {Object} CurrencyProps
Copy link
Member Author

Choose a reason for hiding this comment

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

We can make it

Suggested change
* @typedef {Object} CurrencyProps
* @typedef {NumberConfig} CurrencyConfig

It's valid JSDoc, but TypeScirpt is not able to parse it microsoft/TypeScript#20077

Or we can make it

* @typedef {NumberConfig & { code: string, … } } CurrencyProps

But then we lose definitions of the extra props.

@github-actions
Copy link

This PR has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the status: stale The issue/PR is stale label Dec 25, 2021
@github-actions
Copy link

This PR was automatically closed due to being stale.

@github-actions github-actions bot closed this Dec 30, 2021
@tomalec
Copy link
Member Author

tomalec commented Mar 20, 2022

I'm not sure if we can make it before the merge, but if #7840 lands it would be nice to have this as well.

@tomalec
Copy link
Member Author

tomalec commented Mar 23, 2022

Closing in favor of woocommerce/woocommerce#32337

@tomalec tomalec closed this Mar 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant