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 and expose internals to be used in Vue macro #1976

Merged

Conversation

timofei-iatsenko
Copy link
Collaborator

@timofei-iatsenko timofei-iatsenko commented Jul 12, 2024

Description

extract refactoring to js macro from #1925

Also add a new macro called "arg" which unblock using native ICU syntax directly with macro such as

t`{${arg(count)}, plural, one {}, other {}}`

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 Jul 12, 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 Sep 22, 2024 11:10am

Copy link

github-actions bot commented Jul 12, 2024

size-limit report 📦

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

@timofei-iatsenko timofei-iatsenko changed the title refactor(macro) split and expose internals to be used in Vue macro refactor(macro): split and expose internals to be used in Vue macro Jul 12, 2024
Copy link

codecov bot commented Jul 12, 2024

Codecov Report

Attention: Patch coverage is 93.61702% with 9 lines in your changes missing coverage. Please review.

Project coverage is 80.65%. Comparing base (dd43fb0) to head (b961091).
Report is 104 commits behind head on next.

Files with missing lines Patch % Lines
...ckages/babel-plugin-lingui-macro/src/macroJsAst.ts 91.76% 7 Missing ⚠️
packages/babel-plugin-lingui-macro/src/icu.ts 50.00% 0 Missing and 1 partial ⚠️
...-plugin-lingui-macro/src/messageDescriptorUtils.ts 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1976      +/-   ##
==========================================
+ Coverage   76.66%   80.65%   +3.98%     
==========================================
  Files          81       82       +1     
  Lines        2083     2290     +207     
  Branches      532      604      +72     
==========================================
+ Hits         1597     1847     +250     
- Misses        375      427      +52     
+ Partials      111       16      -95     

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

@timofei-iatsenko timofei-iatsenko marked this pull request as ready for review July 12, 2024 08:18
@andrii-bodnar
Copy link
Contributor

@thekip Thank you! Could you please describe the new arg macro in more detail? What was the motivation and why do we need it?

We should probably decouple exposing internals for Vue integration and the arg feature to not block Vue integration. I would like to avoid merging things that are not yet implemented in the SWC plugin since we already have the useLingui macro which is not supported in the SWC plugin and it blocks the v5 release.

@timofei-iatsenko
Copy link
Collaborator Author

i don't want to split this into two, since this arg macro is also a part of Vue integration but could also be helpful in non Vue code as well.

This macro prints argument placeholder as raw without any additional brackets. This is useful if you want to construct ICU expression manually but still want to use all benefits of macro.

Say i want to construct a plural icu expression without using a plural macro:

// without `arg` macro
t`{${count}, plural, one {}, other {}}`

We will receive a broken icu because of brackets around a count argument:

i18n.t({
  id: "<hash>",
  message: "{{count}, plural, one {}, other {}}",
  values: {count: count}
})

With the arg macro brackets would be omitted:

t`{${arg(count)}, plural, one {}, other {}}`

i18n.t({
   id: "<hash>",
  message: "{count, plural, one {}, other {}}",
  values: {count: count}
})

This is especially useful if you want to create complex ICU expressions and want to use ICU syntax directly.

For Vue this is the only one way to implement Plural functionality in templates. There is no counterpart for <Plural> instead users have to use <Trans> and write ICU expression manually using arg macro.

SWC plugin since we already have the useLingui macro which is not supported in the SWC plugin and it blocks the v5 release.

I don't think useLingui or arg macro are blocking the release. They are not the core functionality and users could decide not to use them if they use SWC version. We can add a "feature parity" table on the SWC plugin page and note which features are implemented and which are not.

Implementing useLingui hook is extremely difficult in Rust actually, that's why I was against this feature originally considering all the consequences of porting it to the SWC

@andrii-bodnar
Copy link
Contributor

Okay, now I get the motivation.

For Vue this is the only one way to implement Plural functionality in templates

We can accept this in the first release and implement the normal Vue Plural later after the release. Let's gather some feedback on the Vue integration first. I think it's the right way instead of creating a new macro that is not fully supported.

Such inconsistencies between Babel and SWC implementations will cause more and more confusion for users. It's better not to release some feature than to release it in half, especially when we have quite a large amount of developers using the SWC plugin. Also, full feature support makes it much easier to write documentation.

@timofei-iatsenko
Copy link
Collaborator Author

timofei-iatsenko commented Aug 1, 2024

We can accept this in the first release and implement the normal Vue Plural later after the release. Let's gather some feedback on the Vue integration first. I think it's the right way instead of creating a new macro that is not fully supported.

There is no way to implement Plural in Vue the same way as in React JSX. So there are no plans to implement a "normal" Plural for Vue.

Such inconsistencies between Babel and SWC implementations will cause more and more confusion for users. It's better not to release some feature than to release it in half, especially when we have quite a large amount of developers using the SWC plugin. Also, full feature support makes it much easier to write documentation.

Would you be interested in contributing to the SWC plugin? Unfortunately, I don't have enough motivation to port features that I don't personally use. Alternatively, you can create a bounty or find a sponsor to support this development effort.

@andrii-bodnar
Copy link
Contributor

andrii-bodnar commented Aug 1, 2024

@JSteunou please share your opinion on this PR and the discussion.

Would you be interested in contributing to the SWC plugin? Unfortunately, I don't have enough motivation to port features that I don't personally use.

I don't have any expertise in Rust. By the way, you already contributed to this feature in #1859 and lingui/swc-plugin#90.

What I'm trying to convey is that such mis-synchronization between Babel and the SWC implementation is not good, and might cause more confusion than the feature brings benefit.

# Conflicts:
#	packages/babel-plugin-lingui-macro/src/macroJs.ts
@timofei-iatsenko
Copy link
Collaborator Author

@andrii-bodnar This one is updated, please review it and consider merging. This makes working on Vue integration easier and PR would have fewer files.

Copy link
Contributor

@andrii-bodnar andrii-bodnar left a comment

Choose a reason for hiding this comment

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

@timofei-iatsenko thank you!

@andrii-bodnar andrii-bodnar merged commit 927c319 into lingui:next Sep 23, 2024
15 of 16 checks passed
@timofei-iatsenko timofei-iatsenko deleted the refactor/expose-js-macro-internals branch September 23, 2024 15:41
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.

2 participants