refactor(macro): robust whitespace handling according to jsx spec #1882

timofei-iatsenko · 2024-03-08T10:19:53Z

Description

related discussion: #1873

Found much better way to process whitespacing.

Instead of merging all different types of JSX nodes (JSXText, JSXExpressionContainer -> StringLiteral, etc) into one big string and then trying to normalize whitespacing in that string i instead utilize nodes information to understand what whitespacing rules should be applied.

In other words:
Before:

<Trans>Hello◦{"◦"}◦world</Trans> 
// -> normalizeWhitespace("Hello◦◦◦world")  we lost here information that middle space came from explicit `{"◦"}`

After

<Trans>Hello◦{"◦"}◦world</Trans>

// there are strict rules how whitespaces processed in JSXText, 
// use `cleanJSXElementLiteralChild` function which follows this rules (taken from babel's sources)
cleanJSXElementLiteralChild("processHello◦") // JSXText
{"◦"} // JSXExpressionContainer - arbitary content, left as is
cleanJSXElementLiteralChild("◦world") // JSXText

So now the processing is reflecting how JSX processes whitespaces

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Examples update

Fixes # (issue)

Checklist

I have read the CONTRIBUTING and CODE_OF_CONDUCT docs
I have added tests that prove my fix is effective or that my feature works
I have added the necessary documentation (if appropriate)

vercel · 2024-03-08T10:19:57Z

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

Name	Status	Preview	Comments	Updated (UTC)
js-lingui	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 19, 2024 9:43am

timofei-iatsenko · 2024-03-08T10:20:59Z

packages/macro/src/macroJs.ts

@@ -52,7 +52,7 @@ export type MacroJsOpts = {
  stripNonEssentialProps: boolean
 }

-export default class MacroJs {
+export class MacroJs {


not related to whitespacing, simply eliminating default import/exports since they are harmful in cjs/esm interops

timofei-iatsenko · 2024-03-08T10:21:26Z

packages/macro/src/macroJsx.ts

-
-  getJsxTagName = (node: JSXElement): string => {
-    if (this.types.isJSXIdentifier(node.openingElement.name)) {
-      return node.openingElement.name.name
-    }
-  }


found not used method

timofei-iatsenko · 2024-03-08T10:21:46Z

packages/macro/src/utils/cleanJSXElementLiteralChild.ts

+export default function cleanJSXElementLiteralChild(value: string) {
+  const lines = value.split(/\r\n|\n|\r/)
+
+  let lastNonEmptyLine = 0


taken as-is from babel's repo

github-actions · 2024-03-08T10:23:47Z

size-limit report 📦

Path	Size
./packages/core/dist/index.mjs	2.86 KB (0%)
./packages/detect-locale/dist/index.mjs	723 B (0%)
./packages/react/dist/index.mjs	1.67 KB (0%)
./packages/remote-loader/dist/index.mjs	7.26 KB (0%)

codecov · 2024-03-08T10:27:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.17%. Comparing base (30b5940) to head (2f8d1db).

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #1882      +/-   ##
==========================================
+ Coverage   76.93%   77.17%   +0.24%     
==========================================
  Files          83       84       +1     
  Lines        2150     2164      +14     
  Branches      556      560       +4     
==========================================
+ Hits         1654     1670      +16     
+ Misses        383      381       -2     
  Partials      113      113

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

timofei-iatsenko · 2024-03-08T11:14:33Z

This is a breaking change, users would need to re-extract and re-translate catalogs due to space changes.

timofei-iatsenko · 2024-03-08T11:43:06Z

packages/macro/test/fixtures/jsx-keep-forced-newlines.expected.js

+import { Trans as _Trans } from "@lingui/react"
+;<_Trans id={"<stripped>"} message={"Keep multiple\nforced\nnewlines!"} />


i extracted this test cases in the test file cases because it was quite hard to understand is escaping working correctly or not when the code is inside js string

vonovak

I like this! But a few questions:

does this mean that babel macro would (further) deviate from SWC implementation?
Would it maybe make sense to make this configurable?

I know it's extra code and extra config, but then it wouldn't be a breaking change and we'd give users ability to choose this new whitespace handler and make it default with the next major version (and hopefully have swc version by then as well?).

Thank you!

vonovak · 2024-03-11T22:29:12Z

packages/macro/test/jsx-trans.ts

@@ -414,35 +414,37 @@ const cases: TestCase[] = [
      `,
  },
  {
-    name: "Keep forced newlines",
+    name: "Strip whitespace around tags but keep whitespaces in JSX containers",


by "JSX container" you mean this? {"Wonderful framework "} - just clarifying what that term means in this context

Yes, this is JSXExpressionContainer according to AST types

timofei-iatsenko · 2024-03-12T10:18:57Z

I'm going to update SWC macro as well. And I would like to avoid making it configurable, the previous whitespace cleaning implementation is simply non-safe and lossy. Depending on the project style there might be little to 0 breaking changes, but for other styles there might be a bit bigger effort needed.

BTW, I'm also going to reconsider whitespace cleaning in core macro.

This white spacing changes would be planned for v5.

There is also possible to write an automatic migration which will convert existing catalogs, but honestly i'm not really interested in that (lack of motivation and time for doing that)

timofei-iatsenko · 2024-03-18T09:56:20Z

About migration, may be some would want to write it, here is the strategy:

Extract messages with old-style whitespace management (fix the specific version of lingui/macro)
Give an uniq id for every message based on filename / line / column (there are such information on the level of catalog)
Extract messages with new version, assign ids, copy translations for ids.
write updated catalogs.

timofei-iatsenko · 2024-03-19T09:34:22Z

@andrii-bodnar could you not a force push a next branch while there are opened PR's. Now it's extremely hard to fix this branch.

andrii-bodnar · 2024-03-19T09:43:59Z

@thekip my bad, forgot about this active PR for the next branch 😬

timofei-iatsenko · 2024-03-19T12:50:43Z

SWC Plugin Port lingui/swc-plugin#83

timofei-iatsenko commented Mar 8, 2024

View reviewed changes

vercel bot deployed to Preview March 8, 2024 10:23 View deployment

vercel bot deployed to Preview March 8, 2024 11:10 View deployment

timofei-iatsenko force-pushed the refactor/jsx-newlines branch from 8f51900 to 4904cde Compare March 8, 2024 11:35

vercel bot deployed to Preview March 8, 2024 11:38 View deployment

timofei-iatsenko mentioned this pull request Mar 8, 2024

lingui extract-experimental bugs #1797

Open

3 tasks

timofei-iatsenko marked this pull request as ready for review March 8, 2024 11:40

timofei-iatsenko commented Mar 8, 2024

View reviewed changes

timofei-iatsenko requested a review from andrii-bodnar March 8, 2024 11:46

andrii-bodnar requested a review from vonovak March 11, 2024 08:58

andrii-bodnar added the ⚠️ breaking change label Mar 11, 2024

vonovak reviewed Mar 11, 2024

View reviewed changes

timofei-iatsenko force-pushed the refactor/jsx-newlines branch from 4904cde to 89eb9c2 Compare March 15, 2024 10:56

vercel bot deployed to Preview March 15, 2024 11:00 View deployment

andrii-bodnar force-pushed the next branch from b6e0656 to 30b5940 Compare March 18, 2024 13:56

refactor(macro): robust whitespace handling according to jsx spec

2f8d1db

timofei-iatsenko force-pushed the refactor/jsx-newlines branch from 89eb9c2 to 2f8d1db Compare March 19, 2024 09:40

vercel bot deployed to Preview March 19, 2024 09:43 View deployment

timofei-iatsenko mentioned this pull request Mar 19, 2024

feature: robust js/jsx whitespace handling lingui/swc-plugin#83

Merged

andrii-bodnar approved these changes Mar 26, 2024

View reviewed changes

andrii-bodnar merged commit 2071e0f into lingui:next Mar 26, 2024
16 checks passed

timofei-iatsenko deleted the refactor/jsx-newlines branch March 28, 2024 09:55

timofei-iatsenko mentioned this pull request Mar 29, 2024

inconsistent message from extract and macro with multiple consecutive newlines / trailing spaces #1799

Closed

timofei-iatsenko mentioned this pull request Jul 9, 2024

Lingui v5 preview - issues with multiline messages #1975

Closed

timofei-iatsenko mentioned this pull request Jul 29, 2024

feat(react): preserve newlines during string formatting #1983

Closed

8 tasks

timofei-iatsenko mentioned this pull request Sep 16, 2024

Lingui ignores whitespace in multi-item Trans tags #525

Closed

andrii-bodnar added this to the v5 milestone Oct 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(macro): robust whitespace handling according to jsx spec #1882

refactor(macro): robust whitespace handling according to jsx spec #1882

timofei-iatsenko commented Mar 8, 2024 •

edited

Loading

vercel bot commented Mar 8, 2024 •

edited

Loading

timofei-iatsenko Mar 8, 2024

timofei-iatsenko Mar 8, 2024

timofei-iatsenko Mar 8, 2024

github-actions bot commented Mar 8, 2024 •

edited

Loading

codecov bot commented Mar 8, 2024 •

edited

Loading

timofei-iatsenko commented Mar 8, 2024 •

edited

Loading

timofei-iatsenko Mar 8, 2024

vonovak left a comment •

edited

Loading

vonovak Mar 11, 2024

timofei-iatsenko Mar 12, 2024

timofei-iatsenko commented Mar 12, 2024

timofei-iatsenko commented Mar 18, 2024 •

edited

Loading

timofei-iatsenko commented Mar 19, 2024 •

edited

Loading

andrii-bodnar commented Mar 19, 2024

timofei-iatsenko commented Mar 19, 2024

		import { Trans as _Trans } from "@lingui/react"
		;<_Trans id={"<stripped>"} message={"Keep multiple\nforced\nnewlines!"} />

refactor(macro): robust whitespace handling according to jsx spec #1882

refactor(macro): robust whitespace handling according to jsx spec #1882

Conversation

timofei-iatsenko commented Mar 8, 2024 • edited Loading

Description

Types of changes

Checklist

vercel bot commented Mar 8, 2024 • edited Loading

timofei-iatsenko Mar 8, 2024

Choose a reason for hiding this comment

timofei-iatsenko Mar 8, 2024

Choose a reason for hiding this comment

timofei-iatsenko Mar 8, 2024

Choose a reason for hiding this comment

github-actions bot commented Mar 8, 2024 • edited Loading

size-limit report 📦

codecov bot commented Mar 8, 2024 • edited Loading

Codecov Report

timofei-iatsenko commented Mar 8, 2024 • edited Loading

timofei-iatsenko Mar 8, 2024

Choose a reason for hiding this comment

vonovak left a comment • edited Loading

Choose a reason for hiding this comment

vonovak Mar 11, 2024

Choose a reason for hiding this comment

timofei-iatsenko Mar 12, 2024

Choose a reason for hiding this comment

timofei-iatsenko commented Mar 12, 2024

timofei-iatsenko commented Mar 18, 2024 • edited Loading

timofei-iatsenko commented Mar 19, 2024 • edited Loading

andrii-bodnar commented Mar 19, 2024

timofei-iatsenko commented Mar 19, 2024

timofei-iatsenko commented Mar 8, 2024 •

edited

Loading

vercel bot commented Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024 •

edited

Loading

codecov bot commented Mar 8, 2024 •

edited

Loading

timofei-iatsenko commented Mar 8, 2024 •

edited

Loading

vonovak left a comment •

edited

Loading

timofei-iatsenko commented Mar 18, 2024 •

edited

Loading

timofei-iatsenko commented Mar 19, 2024 •

edited

Loading