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

refactor(macro): split js and react macro into two packages #1883

Merged
merged 15 commits into from
Mar 15, 2024

Conversation

thekip
Copy link
Collaborator

@thekip thekip commented Mar 8, 2024

Description (edited)

Related RFC: #1361

Split to core (@lingui/core/macro) and react macro (@lingui/react/macro).

And @lingui/macro which re-exports @lingui/core/macro and @lingui/react/macro for backward compatibility. (will be marked as deprecated and would be removed in lingui@7)

Despite an original idea where I wanted to re-export core macro from react, I rejected this because quickly understand that core macro code should be aware of all "re-export" packages so adding macro for Vue, for example, which also re-export core would require adding knowledge of this package to the core.

After this PR typical usage would look like:

import { Trans } from "@lingui/react/macro";
import { t } from "@lingui/core/macro";

function MyComponent() {
  <Trans>Hi, my name is {name}</Trans>
  <span title={t`Title`} />
}

Or with only one import, thanks to useLingui hook:

import { Trans, useLingui } from "@lingui/react/macro";

function MyComponent() {
  const { t } = useLingui();

  <Trans>Hi, my name is {name}</Trans>
  <span title={t`Title`} />
}

This PR also extract the actual transformation code into separate babel plugin which could be used without babel-plugin-macros:

@lingui/babel-plugin-lingui-macro

Next Steps:

  • I probably can create a codemod for automatic import migration, so transition will be smoother.
  • We need to split macro docs to core / react part
  • we need to document @lingui/babel-plugin-lingui-macro and integrate / link it into macro articles
  • we need to update docs examples
  • update SWC version

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)

Copy link

vercel bot commented Mar 8, 2024

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 13, 2024 3:26pm

Copy link

github-actions bot commented Mar 8, 2024

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%)

@thekip thekip changed the title Feature/split js jsx macro @thekip refactor(macro): split js and react macro into two packages Mar 8, 2024
@thekip thekip changed the title @thekip refactor(macro): split js and react macro into two packages refactor(macro): split js and react macro into two packages Mar 8, 2024
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

Attention: Patch coverage is 64.70588% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 76.95%. Comparing base (ead1a61) to head (48838e3).

Files Patch % Lines
packages/vite-plugin/src/index.ts 0.00% 4 Missing ⚠️
packages/babel-plugin-lingui-macro/src/macroJsx.ts 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1883      +/-   ##
==========================================
- Coverage   77.24%   76.95%   -0.29%     
==========================================
  Files          83       83              
  Lines        2153     2144       -9     
  Branches      555      554       -1     
==========================================
- Hits         1663     1650      -13     
- Misses        379      383       +4     
  Partials      111      111              

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

@thekip thekip marked this pull request as ready for review March 8, 2024 16:01
@thekip
Copy link
Collaborator Author

thekip commented Mar 11, 2024

Thought about packages layout during weekend, and come up with that proposition:

  • @lingui/core/macro
  • @lingui/react/macro
  • @lingui/macro - will re-export everything from the two above and would be marked as deprecated for lingui 4.8–5 and deleted completely in lingui 6.

That layout will make this changes less breaking for users and give them enough time to smoothly transition.

Both @lingui/babel-plugin-lingui-macro and babel-plugin-macros are peer dependencies and marked as optional, that means if user going to use only non-macro version of @lingui/core or @lingui/react macro dependencies will not be installed until user will install them manually.

What do you think?

@andrii-bodnar
Copy link
Contributor

  • @lingui/core/macro
  • @lingui/react/macro
  • @lingui/macro - will re-export everything from the two above and would be marked as deprecated for lingui 4.8–5 and deleted completely in lingui 6.

That layout will make this changes less breaking for users and give them enough time to smoothly transition.

Looks ok to me.

But previously you mentioned that re-exporting is not so good idea:

Despite an original idea where I wanted to re-export core macro from react, I rejected this because quickly understand that core macro code should be aware of all "re-export" packages so adding macro for Vue, for example, which also re-export core would require adding knowledge of this package to the core.

@thekip
Copy link
Collaborator Author

thekip commented Mar 11, 2024

Yes, it's not a good idea, but for the transition period it could be re-exported. What i really don't want, to make a coupling between core and react package. core should know nothing about react.

@thekip
Copy link
Collaborator Author

thekip commented Mar 11, 2024

@andrii-bodnar that wouldn't be a breaking change after i implement what i proposed.

@thekip
Copy link
Collaborator Author

thekip commented Mar 11, 2024

@andrii-bodnar any ideas how to update docs?

Option 1.

  • Move macro description to corresponding pages, js -> @lingui/core, jsx to -> @lingui/react
  • @lingui/macro: Put a Notice that this package is deprecated and add to links to the core / react macros
  • create a separate page with installation and macros overview? And make a links from @lingui/core and @lingui/react to that page.

Option2

Left @lingui/macro page as is, but instead rename it to something like "Macro Reference", add additional blocks describing what package to use to import

Your options?

@andrii-bodnar
Copy link
Contributor

@thekip I was thinking about something like this:

Macro Reference > (a section inside the current API Reference)
  - @lingui/core/macro
  - @lingui/react/macro
  - @lingui/vue/macro
  - ...

The "Macro Reference" might have its own page as well with some general description.

@thekip
Copy link
Collaborator Author

thekip commented Mar 12, 2024

The PR is ready to review. I think updating docs better to implement in next PR, since this one is already have 122 files changed

@thekip
Copy link
Collaborator Author

thekip commented Mar 13, 2024

I think codecov just messing up again, this branch hasn't added any new code which is not covered by tests

examples/js/package.json Show resolved Hide resolved
packages/babel-plugin-lingui-macro/package.json Outdated Show resolved Hide resolved
packages/core/package.json Show resolved Hide resolved
packages/core/package.json Show resolved Hide resolved
Copy link
Collaborator

@vonovak vonovak left a comment

Choose a reason for hiding this comment

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

a lot of changes, luckily not too disturbing. 👍

},
"exports": {
".": {
"require": {
Copy link
Collaborator

@vonovak vonovak Mar 15, 2024

Choose a reason for hiding this comment

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

this may (but prob not) require a bit of extra setup in RN. Please treat this as a note to myself, I'll document it in the RN guide if needed.
edit: this really should just work

@andrii-bodnar andrii-bodnar merged commit b6e0656 into lingui:next Mar 15, 2024
16 of 17 checks passed
@thekip thekip deleted the feature/split-js-jsx-macro branch March 15, 2024 08:14
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.

None yet

3 participants