-
-
Notifications
You must be signed in to change notification settings - Fork 441
feat: add optional configurable JSX placeholder naming #2505
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
Changes from 2 commits
66cc180
7aac775
8fcad43
a18eeea
24d1533
ab89d28
c52e5c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,13 +49,18 @@ function maybeNodeValue(node: Node): { text: string; loc: SourceLocation } { | |
| export type MacroJsxContext = MacroJsContext & { | ||
| elementIndex: () => number | ||
| transImportName: string | ||
| elementsTracking: Map<string, JSXElement> | ||
| jsxPlaceholderAttribute?: string | ||
| jsxPlaceholderDefaults?: Record<string, string> | ||
| } | ||
|
|
||
| export type MacroJsxOpts = { | ||
| stripNonEssentialProps: boolean | ||
| stripMessageProp: boolean | ||
| transImportName: string | ||
| isLinguiIdentifier: (node: Identifier, macro: JsMacroName) => boolean | ||
| jsxPlaceholderAttribute?: string | ||
| jsxPlaceholderDefaults?: Record<string, string> | ||
| } | ||
|
|
||
| const choiceComponentAttributesWhitelist = [ | ||
|
|
@@ -86,6 +91,9 @@ export class MacroJSX { | |
| ), | ||
| transImportName: opts.transImportName, | ||
| elementIndex: makeCounter(), | ||
| elementsTracking: new Map(), | ||
| jsxPlaceholderAttribute: opts.jsxPlaceholderAttribute, | ||
| jsxPlaceholderDefaults: opts.jsxPlaceholderDefaults, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -351,18 +359,85 @@ export class MacroJSX { | |
| } | ||
|
|
||
| tokenizeElement = (path: NodePath<JSXElement>): ElementToken => { | ||
| // !!! Important: Calculate element index before traversing children. | ||
| // That way outside elements are numbered before inner elements. (...and it looks pretty). | ||
| const name = this.ctx.elementIndex() | ||
| const { | ||
| jsxPlaceholderAttribute, | ||
| jsxPlaceholderDefaults, | ||
| elementsTracking, | ||
| } = this.ctx | ||
|
|
||
| let node = path.node | ||
| let name: string | undefined = undefined | ||
|
|
||
| if (jsxPlaceholderAttribute) { | ||
| const { attributes } = node.openingElement | ||
| const attrIndex = attributes.findIndex( | ||
| (attr) => | ||
| attr.type === "JSXAttribute" && | ||
| attr.name.name === jsxPlaceholderAttribute, | ||
| ) | ||
|
|
||
| if (attrIndex !== -1) { | ||
| const attr = attributes[attrIndex] as JSXAttribute | ||
| if (attr.value && attr.value.type === "StringLiteral") { | ||
| name = attr.value.value | ||
|
mogelbrod marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| const newAttributes = [...attributes] | ||
| newAttributes.splice(attrIndex, 1) | ||
|
|
||
| node = { | ||
| ...node, | ||
| openingElement: { | ||
| ...node.openingElement, | ||
| attributes: newAttributes, | ||
| }, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!name && jsxPlaceholderDefaults) { | ||
| const tagName = node.openingElement.name | ||
| if (tagName.type === "JSXIdentifier") { | ||
| name = jsxPlaceholderDefaults[tagName.name] | ||
| } | ||
| } | ||
|
|
||
| if (!name) { | ||
| name = String(this.ctx.elementIndex()) | ||
| elementsTracking.set(name, node) | ||
| } else { | ||
| const existingElement = elementsTracking.get(name) | ||
|
|
||
| if (existingElement) { | ||
| const existingAttrs = existingElement.openingElement.attributes | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
<Trans>
<em _t="same">A</em> and <strong _t="same">B</strong>
</Trans>
const props = { href: "/b" }
<Trans>
<a _t="same" {...props} href="/a">A</a>
<a _t="same" href="/a" {...props}>B</a>
</Trans>That is, the React semantics are different. But the current check sees "there is a spread and there is an href" in both cases and considers the elements identical. After this, one of them quietly wins, and both placeholders begin to render identically, even though the original JSX was different.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! Should be addressed in 24d1533
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. However i think this is quite extreme edge case |
||
| const openingAttrs = node.openingElement.attributes | ||
| if ( | ||
| existingAttrs.length !== openingAttrs.length || | ||
| !existingAttrs.every((a) => | ||
| openingAttrs.some((b) => this.types.isNodesEquivalent(a, b)), | ||
| ) | ||
| ) { | ||
| const eg = `(e.g. \`<element ${jsxPlaceholderAttribute || '_t'}="newName" />\`)` | ||
| const msg = `Multiple distinct JSX elements with the same placeholder name (\`${name}\`). ` | ||
| + (jsxPlaceholderAttribute | ||
| ? `Differentiate them by adding/modifying the \`${jsxPlaceholderAttribute}\` attribute ${eg}.` | ||
| : `Differentiate them by setting \`macro.jsxPlaceholderAttribute\` in the lingui config and then adding the attribute to your JSX elements ${eg}.` | ||
| ) | ||
| throw path.buildCodeFrameError(msg) | ||
| } | ||
| } else { | ||
| elementsTracking.set(name, node) | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| type: "element", | ||
| name, | ||
| value: { | ||
| ...path.node, | ||
| ...node, | ||
| children: [], | ||
| openingElement: { | ||
| ...path.node.openingElement, | ||
| ...node.openingElement, | ||
| selfClosing: true, | ||
| }, | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,203 @@ | ||
| // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html | ||
|
|
||
| exports[`Deduplication: Explicit names with different attributes throw an error 1`] = ` | ||
| [SyntaxError: <cwd>/<filename>jsx: Unsupported macro usage. Please check the examples at https://lingui.dev/ref/macro#examples-of-js-macros. | ||
| If you think this is a bug, fill in an issue at https://github.com/lingui/js-lingui/issues | ||
|
|
||
| Error: Multiple distinct JSX elements with the same placeholder name (\`link\`). Differentiate them by adding/modifying the \`_t\` attribute (e.g. \`<element _t="newName" />\`). | ||
| 1 | | ||
| 2 | import { Trans } from '@lingui/react/macro'; | ||
| > 3 | <Trans>Hello <a _t="link" href="/a">link 1</a>, normal, <a _t="link" href="/b">link 2</a>.</Trans> | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| 4 | | ||
| 1 | | ||
| 2 | import { Trans } from '@lingui/react/macro'; | ||
| > 3 | <Trans>Hello <a _t="link" href="/a">link 1</a>, normal, <a _t="link" href="/b">link 2</a>.</Trans> | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| 4 | ] | ||
| `; | ||
|
|
||
| exports[`Deduplication: Identical elements are reused 1`] = ` | ||
| import { Trans } from "@lingui/react/macro"; | ||
| <Trans> | ||
| Hello <em>emphasis</em>, normal, <em>more emphasis</em>. | ||
| </Trans>; | ||
|
|
||
| ↓ ↓ ↓ ↓ ↓ ↓ | ||
|
|
||
| import { Trans as _Trans } from "@lingui/react"; | ||
| <_Trans | ||
| { | ||
| /*i18n*/ | ||
| ...{ | ||
| id: "idxihm", | ||
| message: "Hello <em>emphasis</em>, normal, <em>more emphasis</em>.", | ||
| components: { | ||
| em: <em />, | ||
| }, | ||
| } | ||
| } | ||
| />; | ||
|
|
||
| `; | ||
|
|
||
| exports[`Deduplication: Identical elements with different prop order are reused 1`] = ` | ||
| import { Trans } from "@lingui/react/macro"; | ||
| <Trans> | ||
| Hello{" "} | ||
| <a _t="link" href="/a" class="foo"> | ||
| link 1 | ||
| </a> | ||
| , normal,{" "} | ||
| <a _t="link" class="foo" href="/a"> | ||
| link 1 copy | ||
| </a> | ||
| . | ||
| </Trans>; | ||
|
|
||
| ↓ ↓ ↓ ↓ ↓ ↓ | ||
|
|
||
| import { Trans as _Trans } from "@lingui/react"; | ||
| <_Trans | ||
| { | ||
| /*i18n*/ | ||
| ...{ | ||
| id: "9en3MH", | ||
| message: "Hello <link>link 1</link>, normal, <link>link 1 copy</link>.", | ||
| components: { | ||
| link: <a class="foo" href="/a" />, | ||
| }, | ||
| } | ||
| } | ||
| />; | ||
|
|
||
| `; | ||
|
|
||
| exports[`Deduplication: Implicit names with distinct attributes throw an error 1`] = ` | ||
| [SyntaxError: <cwd>/<filename>jsx: Unsupported macro usage. Please check the examples at https://lingui.dev/ref/macro#examples-of-js-macros. | ||
| If you think this is a bug, fill in an issue at https://github.com/lingui/js-lingui/issues | ||
|
|
||
| Error: Multiple distinct JSX elements with the same placeholder name (\`a\`). Differentiate them by setting \`macro.jsxPlaceholderAttribute\` in the lingui config and then adding the attribute to your JSX elements (e.g. \`<element _t="newName" />\`). | ||
| 1 | | ||
| 2 | import { Trans } from '@lingui/react/macro'; | ||
| > 3 | <Trans>Hello <a href="/a">link 1</a>, normal, <a href="/b">link 2</a>.</Trans> | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^ | ||
| 4 | | ||
| 1 | | ||
| 2 | import { Trans } from '@lingui/react/macro'; | ||
| > 3 | <Trans>Hello <a href="/a">link 1</a>, normal, <a href="/b">link 2</a>.</Trans> | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| 4 | ] | ||
| `; | ||
|
|
||
| exports[`Deduplication: Same explicit placeholder with identical attributes does not throw 1`] = ` | ||
| import { Trans } from "@lingui/react/macro"; | ||
| <Trans> | ||
| Hello{" "} | ||
| <a _t="link" href="/a"> | ||
| link 1 | ||
| </a> | ||
| , normal,{" "} | ||
| <a _t="link" href="/a"> | ||
| link 1 copy | ||
| </a> | ||
| . | ||
| </Trans>; | ||
|
|
||
| ↓ ↓ ↓ ↓ ↓ ↓ | ||
|
|
||
| import { Trans as _Trans } from "@lingui/react"; | ||
| <_Trans | ||
| { | ||
| /*i18n*/ | ||
| ...{ | ||
| id: "9en3MH", | ||
| message: "Hello <link>link 1</link>, normal, <link>link 1 copy</link>.", | ||
| components: { | ||
| link: <a href="/a" />, | ||
| }, | ||
| } | ||
| } | ||
| />; | ||
|
|
||
| `; | ||
|
|
||
| exports[`Mixing explicit _t together with jsxPlaceholderDefaults 1`] = ` | ||
| import { Trans } from "@lingui/react/macro"; | ||
| <Trans> | ||
| Hello <a href="/a">link 1</a>, normal,{" "} | ||
| <a _t="link2" href="/b"> | ||
| link 2 | ||
| </a> | ||
| . | ||
| </Trans>; | ||
|
|
||
| ↓ ↓ ↓ ↓ ↓ ↓ | ||
|
|
||
| import { Trans as _Trans } from "@lingui/react"; | ||
| <_Trans | ||
| { | ||
| /*i18n*/ | ||
| ...{ | ||
| id: "yU9TUm", | ||
| message: "Hello <link>link 1</link>, normal, <link2>link 2</link2>.", | ||
| components: { | ||
| link: <a href="/a" />, | ||
| link2: <a href="/b" />, | ||
| }, | ||
| } | ||
| } | ||
| />; | ||
|
|
||
| `; | ||
|
|
||
| exports[`Placeholder attribute is stripped from AST 1`] = ` | ||
| import { Trans } from "@lingui/react/macro"; | ||
| <Trans> | ||
| <a _t="link" href="/about"> | ||
| About | ||
| </a> | ||
| </Trans>; | ||
|
|
||
| ↓ ↓ ↓ ↓ ↓ ↓ | ||
|
|
||
| import { Trans as _Trans } from "@lingui/react"; | ||
| <_Trans | ||
| { | ||
| /*i18n*/ | ||
| ...{ | ||
| id: "Ym2S6K", | ||
| message: "<link>About</link>", | ||
| components: { | ||
| link: <a href="/about" />, | ||
| }, | ||
| } | ||
| } | ||
| />; | ||
|
|
||
| `; | ||
|
|
||
| exports[`Respects jsxPlaceholderDefaults 1`] = ` | ||
| import { Trans } from "@lingui/react/macro"; | ||
| <Trans> | ||
| Here's a <a>link</a> and <em>emphasis</em>. | ||
| </Trans>; | ||
|
|
||
| ↓ ↓ ↓ ↓ ↓ ↓ | ||
|
|
||
| import { Trans as _Trans } from "@lingui/react"; | ||
| <_Trans | ||
| { | ||
| /*i18n*/ | ||
| ...{ | ||
| id: "Jg_WOt", | ||
| message: "Here's a <link>link</link> and <em>emphasis</em>.", | ||
| components: { | ||
| link: <a />, | ||
| em: <em />, | ||
| }, | ||
| } | ||
| } | ||
| />; | ||
|
|
||
| `; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a custom placeholder attribute is present but its value is not a non-empty string literal, such as
_t={linkName}or_t="",this code still removes the attribute from the JSX and then silently falls back to the auto-generated numeric placeholder.
Throw an error?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Should be addressed in a18eeea
Please resolve this thread if you think the solution covers the issue @yslpn 😃