feat(macro): make message stripping configurable via Babel options

[toblu]

Description

Make it possible to configure whether the message prop should be stripped or not from the transpiled output. I have added a new option called stripMessageField that can be set from the Babel config to control the behaviour.

This does not change any of the default behaviour in the macro. Message (and other props like comment / context) will be kept in development mode, but removed in production mode (NODE_ENV ===" production") by default. But it is now possible to override this behaviour by setting stripMessageField option in Babel config:

Example babel.config.js

// babel.config.js
module.exports = {
    plugins: [
        "macros",
        {
            // ...
            // Other macros config
            lingui: {
                // message field will be removed in both dev and prod mode
                stripMessageField: true,
            },
        }
    ]
}

The section below outlines the different behaviours based on the stripMessageField option:

• Default (not set): message prop will be kept in development mode, but removed in production (when process.env.NODE_ENV === "production")
• stripMessageField: true: always removes the message prop except during the extraction process
• stripMessageField: false: never removes the message prop

Motivation for the change

We have recently added Lingui to our project and are so far very happy with it. One issue that we have encountered though is discrepancy between development mode and production mode due to how message fallbacks work. In development mode, the macro will fallback to the source message if there is no translated message available in the .po file (e.g. if the message hasn't been extracted) or if there is an issue with how the macro is used (e.g. using t macro directly on module level). However, in production mode the message prop is removed by default and therefore fallbacks to showing the ID in these cases.

For our workflow, we would like development mode to behave more like production and NOT fallback to the source message in the cases above to give developers an earlier indication that something is not working as it should during the development phase. On the contrary, one could also use stripMessageField: false to allow fallback to source message in production if a translated message is not available (possibly related to ##2043).

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]

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

Name	Status	Preview	Updated (UTC)
js-lingui	✅ Ready (Inspect)	Visit Preview	Oct 23, 2024 6:55am

[github-actions]

size-limit report 📦

Path	Size
./packages/core/dist/index.mjs	3.09 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%)

[toblu]

I have not added any documentation for the new option, but I would be happy to help provide documentation for it if this is an acceptable solution

[timofei-iatsenko]

Many thanks for you contribution, you did something that I had on my radar for a long time and was going to do in the nearest future.

We are very close to release a new v5 version and don't want to merge new features into the stable v4 branch. Please rebase it to the next branch.

Regarding the documentation, I could not imagine a good place where it could be added. I have a few ideas for improvement for documentation for now, so probably i will document it on my own, later, as part of that work.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.72%. Comparing base (dd43fb0) to head (542875f).
Report is 137 commits behind head on next.

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #2054      +/-   ##
==========================================
- Coverage   76.66%   75.72%   -0.95%     
==========================================
  Files          81       85       +4     
  Lines        2083     2323     +240     
  Branches      532      611      +79     
==========================================
+ Hits         1597     1759     +162     
- Misses        375      450      +75     
- Partials      111      114       +3     

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

[toblu]

Many thanks for you contribution, you did something that I had on my radar for a long time and was going to do in the nearest future.

We are very close to release a new v5 version and don't want to merge new features into the stable v4 branch. Please rebase it to the next branch.

Regarding the documentation, I could not imagine a good place where it could be added. I have a few ideas for improvement for documentation for now, so probably i will document it on my own, later, as part of that work.

You are most welcome and thank you for all your work maintaining this great project! :)

Sounds like a good idea to include it in v5 if it is just around the corner anyway, I have rebased to the next branch now.

[timofei-iatsenko]

i think this is not needed, because this case is not possible. The babel options which extractor uses are not exposed to the userland and could not be overridden by users. So i could not imagine the situation when {extract: true, stripMessageProp: true} would be passed together.

This will allow to delete some of the tests, less code, less things to maintain.

Thanks!

[toblu]

Well spotted! I added it in the initial implementation based on v4. In that version when I ran the extract command the message was stripped if I had stripMessageProp: true set in the Babel config unless I had this line to explicitly return false during extraction.

I tested it now against the new v5 implementation and it indeed seems like the stripMessageField: true option does not affect extract. However if I remove this check, some of the existing test cases will break. For example this one since it will remove the message prop due to production: true:

js-lingui/packages/babel-plugin-lingui-macro/test/js-t.test.ts

Lines 208 to 222 in 3a51350

	name: "Production - all props kept if extract: true",
	production: true,
	macroOpts: {
	extract: true,
	},
	code: `
	import { t } from '@lingui/core/macro';
	const msg = t({
	message: \`Hello $\{name\}\`,
	id: 'msgId',
	comment: 'description for translators',
	context: 'My Context',
	})
	`,
	},

How do you think it should be handled?

One alternative that I see could be to change the implementation to something like:

function shouldStripMessageProp(opts: LinguiPluginOpts) {
  if (typeof opts.stripMessageField === "boolean") {
    // if explicitly set in options, use it
    return opts.stripMessageField
  }
  // default to strip message in production if no explicit option is set and not during extract
  return process.env.NODE_ENV === "production" && !opts.extract

This way it would be more aligned with the stripNonEssentialProps value and won't mess up extract if someone runs extract with NODE_ENV set to "production".

[timofei-iatsenko]

Well spotted! I added it in the initial implementation based on v4. In that version when I ran the extract command the message was stripped if I had stripMessageProp: true set in the Babel config unless I had this line to explicitly return false during extraction.

That's strange, because nothing was changed between 4 and 5 in that regard. Lingui extractor does not use user's babel config (intentionally) and no options from there could affect extraction process.

Your proposed alternative is fine.

[toblu]

It might have been me that messed something up when I tested it before, as I was testing a few different implementations in v4. Anyway I confirmed in v5 that it doesn't seem to be a possible case like you said. I have changed the implementation now to what I proposed in my previous comment since that doesn't break any of the existing test cases.

[vonovak]

Aligning dev and prod behavior more closely is definitely a step in the right direction!

Should stripMessageField: true(=aligning prod and dev more closely) become the default in v5?

[timofei-iatsenko]

Should stripMessageField: true(=aligning prod and dev more closely) become the default in v5?

It would be quite a bold decision, because it will ruin DX in dev mode out of the box. I would rather suggest to start from more clear and concise docs so that people will understand the differences.

Upd:

Other good option would be make stripMessageField: false on both development and production, and make stripping behavior opt-in. But this is out of scope of this PR.