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

fix: JSX attribute parsing issue when using double quotes #1226

Merged
merged 3 commits into from
Apr 24, 2022

Conversation

skovhus
Copy link
Contributor

@skovhus skovhus commented Apr 5, 2022

This was the least intrusive way I could come up with by wrapping the relevant jsxAttribute string literal value in a jsxExpressionContainer.

This is currently done conditionally in case the message includes a double quote. Note that I believe this could safely be done unconditionally, but it requires updating a lot of tests. I happily do this if we like the suggested solution.

This has been tested out on a fairly big codebase.

Quoted JSX attributes use XML-style escapes instead of JavaScript-style escapes.
@vercel
Copy link

vercel bot commented Apr 5, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/lingui-js/js-lingui/4b45GDoiHUkZoH333z3nYLrhfk66
✅ Preview: Failed

@skovhus skovhus changed the title Fix JSX attribute parsing issue when using double quotes fix: JSX attribute parsing issue when using double quotes Apr 5, 2022
@codecov
Copy link

codecov bot commented Apr 5, 2022

Codecov Report

Merging #1226 (8828750) into main (0e270a3) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1226      +/-   ##
==========================================
+ Coverage   81.66%   81.69%   +0.03%     
==========================================
  Files          56       56              
  Lines        1767     1770       +3     
  Branches      490      491       +1     
==========================================
+ Hits         1443     1446       +3     
  Misses        194      194              
  Partials      130      130              
Impacted Files Coverage Δ
packages/macro/src/macroJsx.ts 91.36% <100.00%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e270a3...8828750. Read the comment docs.

@skovhus
Copy link
Contributor Author

skovhus commented Apr 7, 2022

@semoal Note that I've been testing this out on a fairly big project and haven't identified any issues.

@skovhus
Copy link
Contributor Author

skovhus commented Apr 8, 2022

Let me know if I can help fixing the Vercel CI error.

@vercel
Copy link

vercel bot commented Apr 20, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
js-lingui ❌ Failed (Inspect) Apr 20, 2022 at 0:16AM (UTC)

// Quoted JSX attributes use XML-style escapes instead of JavaScript-style escapes.
// This means that <Trans id="Say \"hi\"!" /> is invalid, but <Trans id={"Say \"hi\"!"} /> is valid.

// We could consider removing this condition and always wrap in a jsxExpressionContainer.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the conditional here seems safe – I've done this in a larger codebase.

Let me know if we should remove the conditional here (a lot of the tests would need to be updated).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@semoal Let me know if we should remove the conditional here (a lot of the tests would need to be updated).

Copy link
Contributor

Choose a reason for hiding this comment

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

Will that imply a breaking change? Sorry but I don't get why we should update the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will not introduce a breaking change, but as all id and and message attribute will now be wrapped in an expression, then all the tests would need to be updated.

So instead of <Trans id="test" /> it would be <Trans id={"test"} />. Again not breaking change.

Keeping the conditional might be a fine tradeoff, then we only wrap the attributes in an expression in case there is a " in them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should remove the conditional here to handle additional corner cases. This was done in #1234

@semoal
Copy link
Contributor

semoal commented Apr 24, 2022

Super cool, thanks for the contribution mate

@semoal semoal merged commit 27a7ded into lingui:main Apr 24, 2022
@skovhus skovhus deleted the ks/fix-jsx-quotes branch April 24, 2022 20:02
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.

Quoted JSX attributes use XML-style escapes
2 participants