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

[docs-infra] Move ApiPage to TS #43149

Merged
merged 11 commits into from
Aug 2, 2024
Merged

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Aug 2, 2024

Extracted from #43128

This PR can be read commit by commit

I tried to move useTranslate from (key, options) => any to (key, options) => string. But some part of the codebase rely on it returning null is the key translation is not defined (the API pages titles) and (key, options) => string | null leads to a lot of place where t('...') needs to be marked as non-null t('...')!. SO I dropped this idea

@alexfauquette alexfauquette added scope: docs-infra Specific to the docs-infra product typescript labels Aug 2, 2024
@mui-bot
Copy link

mui-bot commented Aug 2, 2024

Netlify deploy preview

https://deploy-preview-43149--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 6e30808

@Janpot
Copy link
Member

Janpot commented Aug 2, 2024

But some part of the codebase rely on it returning null is the key translation is not defined (the API pages titles) and (key, options) => string | null leads to a lot of place where t('...') needs to be marked as non-null t('...')!.

Perhaps it could make sense to have two complementary functions? One that throws and one that returns null when a translation doesn't exist. The throwing one can then have return type string. This is a bit like the react testing library approach of using getBy vs. queryBy. In terms of implementation, one can be built on top of the other, so no duplication required. A potential API:

const t = useTranslate()

const maybeString: string | null = t('abc')
const certainlyString: string = t.require('xyz')

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Looks all good to me, everything strongly typed. Left one suggestion about solving the types of the translation function

@alexfauquette alexfauquette merged commit 85a3b55 into mui:next Aug 2, 2024
22 checks passed
@alexfauquette alexfauquette deleted the api-to-ts-2 branch August 2, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: docs-infra Specific to the docs-infra product typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants