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

Update macro to wrap defaults in brackets when necessary #1472

Merged
merged 2 commits into from
Mar 25, 2022

Conversation

heyimalex
Copy link
Contributor

@heyimalex heyimalex commented Mar 24, 2022

I noticed that the Trans macro generates invalid code when children contains a double quote. From ast explorer:

// before
<Trans>"</Trans>;
// after
<Trans defaults="\"" components={[]} values={{}} />;

According to the JSX spec this isn't valid; JSX doesn't support any kind of escaping inside of " quoted attributes. Babel, tsc, prettier, etc all fail when parsing. To get around this we can just wrap the value in brackets. I made it only happen when the string in question contains a quote.

// before
<Trans>"</Trans>;
// after
- <Trans defaults="\"" components={[]} values={{}} />;
+ <Trans defaults={"\""} components={[]} values={{}} />;

I am kinda confused about how this is working today without issue.

I think we could also defaults.replace(/"/g, "&quot;") instead if that's preferred.

Checklist

  • only relevant code is changed (make a diff before you submit the PR)
  • run tests npm run test
  • tests are included

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 96.078% when pulling 1b8aefa on heyimalex:fix-macro-unescaped into 8e1e852 on i18next:master.

@adrai adrai requested a review from jamuhl March 25, 2022 05:50
Copy link
Member

@jamuhl jamuhl left a comment

Choose a reason for hiding this comment

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

Don't know the specs in detail but assumed we can just escape...guess this is ok...

@adrai adrai merged commit a7af333 into i18next:master Mar 25, 2022
@adrai
Copy link
Member

adrai commented Mar 25, 2022

It's included in v11.16.2

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

Successfully merging this pull request may close these issues.

4 participants