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

Allow missing translations to be compiled/interpolated on production #2012

Closed
garikkh opened this issue Aug 22, 2024 · 9 comments
Closed

Allow missing translations to be compiled/interpolated on production #2012

garikkh opened this issue Aug 22, 2024 · 9 comments

Comments

@garikkh
Copy link
Contributor

garikkh commented Aug 22, 2024

Is your feature request related to a problem? Please describe.

Currently, translations that are not extracted have issues when rendering on production. This only happens when NODE_ENV=production. There are two main issues:

  1. the message attribute is removed on production. This seems to be intentional, and can be fixed by configuring the babel macro plugin, but it is really not clear and maybe just needs to be documented better
  2. Values that are interpolated will NOT get rendered properly - the interpolation does not happen because of a NODE_ENV check prevents the message to be compiled properly.

related check

With this, you are forced to extract and compile when doing any production build in your JS package. Although many people probably do, I think its an unnecessary restriction on the library.


Describe proposed solution

A configuration option (probably in the actual i18n object) that allows for compiling the translation on production.


Describe alternatives you've considered

I know that @lingui/macro has an option called extract that keeps the default message as a parameter when creating the i18n(...) call, however this isn't enough because it only works with basic strings. Strings that need to be interpolated will not be correctly interpolated, and placeholder variables will show in production.

Without macro option:
image

With the macro option:

image


Additional context
Add any other context or screenshots about the feature request here.

@garikkh garikkh changed the title Allow for Allow missing translations to be compiled/interpolated on production Aug 22, 2024
@garikkh
Copy link
Contributor Author

garikkh commented Aug 24, 2024

Added option here: main...garikkh:js-lingui:feat/prod-compile-opt

@timofei-iatsenko
Copy link
Collaborator

timofei-iatsenko commented Aug 26, 2024

Yes, this intentional by design.

Added option here: main...garikkh:js-lingui:feat/prod-compile-opt

This will break tree shaking for message compiler.

I believe making it configurable brings more maintenance burden and confusing for people than benefits.

Step back and let's analyze WHY you need this. I bet it's because you are struggling with proper place for extraction command, so sometimes something don't get to catalogs.

Try to put an additional extraction command just before build command:

build: "lingui extract && app build"

This would work as a last resort if something was not extracted / translated and you will not see ids in the production

@garikkh
Copy link
Contributor Author

garikkh commented Aug 26, 2024

This will break tree shaking for message compiler.

Not sure what you mean by this, this is the runtime, why would it affect tree shaking.

I bet it's because you are struggling with proper place for extraction command, so sometimes something don't get to catalogs.

I understand its common for people to be extracting and compiling as part of the build process, as I mentioned in the issue, its just something I don't want to introduce to my system.

I don't want to build a reliance on translation files and my code. My code should be able to be built and deployed independently of what the translation catalog has.

Is there anything functionally wrong with the approach I have outlined? If it's just a matter of having extra attributes at runtime, I think that's a tradeoff I'd be willing to make.

@timofei-iatsenko
Copy link
Collaborator

Not sure what you mean by this, this is the runtime, why would it affect tree shaking.

The core module is written in the way that in production message compiler is completely removed from the bundle. This is achieved by writing the code which is analyzable during compile time by most tree shakers. Introducing a condition dependent on a runtime flag will break this behavior, since tree-shaker will not be able to figure out value of this flag during build.

This is a solvable issue, I just outlined that your code will break what we already have and expect from the library.

I don't want to build a reliance on translation files and my code. My code should be able to be built and deployed independently of what the translation catalog has.

I completely agree with you, code should be ready to use disregard the state of the translation files, that's why I propose you

lingui extract && app build

Is there anything functionally wrong with the approach I have outlined? If it's just a matter of having extra attributes at runtime, I think that's a tradeoff I'd be willing to make.

This creates a whole new matrix of variations and deviations. So finding an issue and helping people would become harder.

If it's just a matter of having extra attributes at runtime

Actually it's not, there would be a lot more extra attributes. Macro dropping message,comment and context fields, they all could have a very long content.

Also, another problem which actually exists now as well, is that Plural Rules are used for selected language (say PL with 4 forms). When you don't have a translation and fallback to default language (en with 2 forms) plural rules may not match (and most likely won’t). Right now it's an exceptional case because lingui forces to be everything translated and compiled, but if you relax this rule, that problem might appear.

@garikkh
Copy link
Contributor Author

garikkh commented Aug 29, 2024

Thank you for your feedback and I appreciate the detail.

I will close this issue now. Sadly I'll have to keep my changes as a patch in my build. We unfortunately cannot be building and compiling translation catalog during our production builds (yet).

I understand there is a performance concern with both the compiler and the extra message and comment fields in the production JS. This is just something we'll have to deal with until we can change our production build process. Webpack chunking should be effective at keeping the js files small enough anyways.

@garikkh garikkh closed this as completed Aug 29, 2024
@timofei-iatsenko
Copy link
Collaborator

You can workaround the issue by overriding i18n instance to your patched version, and setup a macro with extract: true option.

@garikkh
Copy link
Contributor Author

garikkh commented Sep 4, 2024

@thekip Yes, I have to keep the patched version for my project. I prefer to not have patches, but I understand that its not in your scope to support it

@timofei-iatsenko
Copy link
Collaborator

timofei-iatsenko commented Sep 5, 2024

Actually i'm thinking of adding an extra method to register a compiler.

Something like that:

const i18n = initializeI18n();

i18n.setCompiler(compiler);

The same method would be used under the hood for NODE_ENV === "development" condition.

So this will be tree-shaker compatible and allow this to be configured.

In the next iteration it's possible to make this conditional explicit in the userland code, to increase awareness.

@timofei-iatsenko
Copy link
Collaborator

Started working on that feature in this PR #2035

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

No branches or pull requests

2 participants