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

Mess in exported Macro Typescript Typings #1323

Closed
timofei-iatsenko opened this issue Jan 6, 2023 · 4 comments · Fixed by #1340
Closed

Mess in exported Macro Typescript Typings #1323

timofei-iatsenko opened this issue Jan 6, 2023 · 4 comments · Fixed by #1340
Assignees
Milestone

Comments

@timofei-iatsenko
Copy link
Collaborator

timofei-iatsenko commented Jan 6, 2023

I'm continuing digging into the sourcecode and test suite. Here what i've found:

According to the exported typings all JSX Macro components (Trans, Plural, etc) have common Trans props, which is following:

export type TransProps = {
  id?: string
  comment?: string
  values?: Record<string, unknown>
  context?: string
  children?: React.ReactNode
  component?: React.ComponentType<TransRenderProps>
  render?: (props: TransRenderProps) => ReactElement<any, any> | null
  i18n?: I18n
}

export type ChoiceProps = {
  value?: string | number
} & TransProps &
  ChoiceOptions<ReactNode>

export type SelectProps = {
  value: string
  other: ReactNode
} & TransProps & Values

export const Trans: ComponentType<TransProps>
export const Plural: ComponentType<ChoiceProps>
export const Select: ComponentType<SelectProps>
export const SelectOrdinal: ComponentType<ChoiceProps>

This is actually wrong, because JSX Macro (from the source code and tests) could accept only this set properties:

export type TransProps = {
  id?: string
  comment?: string
-  values?: Record<string, unknown>
  context?: string
-  children?: React.ReactNode // should be only for Trans, not by any other Choice components
-  component?: React.ComponentType<TransRenderProps>
  render?: (props: TransRenderProps) => ReactElement<any, any> | null
  i18n?: I18n // this one is not documented any where, we should add it to the docs
}

So passing any of the properties highlihted in the snippet above will not work. These typings just bring mess and confusing for end users. (it's actually props of original Trans runtime component, not a macro).

Proofs: https://github.com/lingui/js-lingui/blob/main/packages/macro/src/macroJsx.ts#L179-L194
Other properties would stripped or inlined as choice options in ICU Choice component.

Am i missing something or it's here by historical reasons and a just need to be cleaned up wisely?

@timofei-iatsenko
Copy link
Collaborator Author

Related: #1288

Select typings are wrong.

@tricoder42
Copy link
Contributor

Am i missing something or it's here by historical reasons and a just need to be cleaned up wisely?

No, you're right. Trans macro has fewer props that Trans component. values, component are generated by macro itself, so you should use them explicitly.

// Macro
<Trans>Hello <strong>{name}</strong></Trans>

// Component
<Trans
  id="Hello <0>{name}</0>"
  values={{ name }}
  components={[<strong />]}
/>

Howerver, children seems perfectly valid for macro as well. I would even argue that children prop does make sense only to macro, not component, where you'd you id prop instead:

// Macro
<Trans>Hello world</Trans>

// Component
<Trans id="Hello world" />

In the end, separating macro props from component props seems to be a good idea.

@timofei-iatsenko
Copy link
Collaborator Author

Children is valid only for macro, not for Plural / SelectOrdinal / Select
That I had in mind

@andrii-bodnar andrii-bodnar added this to the v4 milestone Feb 13, 2023
@Martin005 Martin005 linked a pull request Feb 14, 2023 that will close this issue
@Martin005
Copy link
Contributor

Typings fixed in #1340 which will be a part of v4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants