Fix conflicting strings issue in translations #917
Conversation
|
Deploy preview for docusaurus-preview ready! Built with commit cf1405c |
lib/write-translations.js
Outdated
| translations['localized-strings'] = Object.assign( | ||
| translations['localized-strings'], | ||
| customTranslations['localized-strings'] | ||
| translations['localized-strings'].links = Object.assign( |
There was a problem hiding this comment.
Any reason not to simply deepmerge ?
IMHO using Object assign for one level deep and deepmerge for > one level deep is not as declarative as simply deepmerging
translations['localized-strings'] = deepmerge(
translations['localized-strings'],
customTranslations['localized-strings']
);
lib/core/DocsLayout.js
Outdated
| this.props.metadata.localized_id | ||
| ] || this.props.metadata.title | ||
| : this.props.metadata.title; | ||
| ? (i18n['localized-strings'].docs[id] && |
There was a problem hiding this comment.
Seeing this made me realize that we should create a safe getter to access deeply nested object.
We don't want to keep doing this. Example:
// access deeply nested values...
translation &&
translation[this.props.metadata.language] &&
translation[this.props.metadata.language].docs &&
translation[this.props.metadata.language].docs[id]We can create a small helper function (maybe in core/utils.js)
const idx = (target, path) =>
path.reduce((obj, key) => (obj && obj[key] ? obj[key] : null), target);So we can write
const title = idx(i18n, ['localized-strings', 'docs', id, 'title']) || defaultTitle;instead of
const title = i18n ? (i18n['localized-strings'].docs[id] && i18n['localized-strings'].docs[id].title) || defaultTitle : defaultTitle;WDYT ?
There was a problem hiding this comment.
I used almost the same method you used to access a deep property, but modified it so that the function accepts the path as a string rather than an array, because most of the libraries out there follow the same pattern.
If we need to be more safe then we can move to any library like just-safe-get.
lib/core/DocsLayout.js
Outdated
| ] || this.props.metadata.title | ||
| : this.props.metadata.title; | ||
| const title = | ||
| utils.getDeepProp(i18n, `localized-strings.docs.${id}.title`) || |
There was a problem hiding this comment.
Note that id can be in the form of string with dot inside. Example: version-1.0.0-hello-world
If we are splitting by ., this could cause a bug
There was a problem hiding this comment.
Good catch, will fix it!
There was a problem hiding this comment.
I am still in favor of using array. It is more declarative to me. The dot notation will only work if there is no key that contains a dot.
This is why we have to do props['hello.world'] sometimes because props.hello.world wont work
Compare
['localized-string', id] vs 'localized-string.${id}'
In fact just-safe-get library you mentioned also support accessing deep nested pros using array path
import get from 'just-safe-get';
const obj = {a: {aa: {aaa: 2}}, b: 4};
get(obj, 'a.aa.aaa'); // 2
get(obj, ['a', 'aa', 'aaa']); // 2There was a problem hiding this comment.
But we dont need a library for that. The idx function implementation I mentioned in last review should be quite ok.
The naming of idx is because Facebook internally have that function.
https://facebook.github.io/react-native/blog/2017/03/13/idx-the-existential-function
But it requires babel plugin 😂
https://github.com/facebookincubator/idx
There was a problem hiding this comment.
I fixed and renamed it back to idx!
Edited it a little bit to return actual value instead of returning null on every falsy value.
|
@endiliey Can you do a new release real quick? |
Fixes #916
Fixes #324
Motivation
Initially experienced at babel/website#1770 (comment), later worked along with @endiliey on Discord room to find the issues and work on a fix. The main issues are explained in #916.
Have you read the Contributing Guidelines on pull requests?
Yes I did
Test Plan
Tested on sample repo, to work as expected as explained in #916
Related PRs
This PR changes the way customized localization strings are handled. Existing setups should not break that bad, but the new translations are to be used as explained in the docs.