fix(core): Do not select plurals for non-top level compilations - #362
dpchamps wants to merge 1 commit into
Conversation
Sorry, something went wrong.
eemeli
left a comment
There was a problem hiding this comment.
Looks promising! Recommended a slightly alternative approach below. The docs may need a small update as well?
Sorry, something went wrong.
| const pl = isDescendentCall | ||
| ? plural | ||
| : (plurals && plurals[key]) || plural; | ||
| result[key] = this.compile(src[key], pl, plurals, true); |
There was a problem hiding this comment.
| const pl = isDescendentCall | |
| ? plural | |
| : (plurals && plurals[key]) || plural; | |
| result[key] = this.compile(src[key], pl, plurals, true); | |
| const pk = plurals && plurals[key]; | |
| result[key] = pk | |
| ? this.compile(src[key], pk) | |
| : this.compile(src[key], pl, plurals); |
Wouldn't that have the same effect, with no need for an additional argument?
Sorry, something went wrong.
There was a problem hiding this comment.
Hi @eemeli , sorry for the delayed response!
Yes, I believe this would work. It's slightly different in that it supports locale keys at any depth... I didn't realize at first, but your feedback on the other PR leads me to believe that this is a documented feature?
In other words, should the following specification pass?
it('should produce the correct output given nested sub keys that look like plurals', () => {
const mf = new MessageFormat('*');
const messagePacks = {
someGrouping: {
es: {
en: 'Estimation Date: {date, date, ::EEEMMMd}',
fi: 'Financial Incentive Date: {date, date, ::EEEMMMd}'
}
},
};
const result = compileModule(mf, messagePacks);
expect(result).toMatch(`new Intl.DateTimeFormat("es", opt);`);
expect(result).not.toMatch(`new Intl.DateTimeFormat("fi", opt);`);
expect(result).not.toMatch(`new Intl.DateTimeFormat("en", opt);`);
});If yes then we should take your suggestions.
Sorry, something went wrong.
There was a problem hiding this comment.
@dpchamps Sorry, completely dropped the ball on this. Yes, locale keys may be set at any depth. For instance, you could have a structure like this, and it should work:
{ foo: { bar: { en: 'Bar in English', fi: 'Baari suomeksi' } } }
Sorry, something went wrong.
There was a problem hiding this comment.
@eemeli Now I've gotta apologize! To be quite honest, I don't remember the context for this fix, so I'll have to set aside some time for focus.
Are you still interested in landing this patch? If so, I can take a look sometime this week.
Sorry, something went wrong.
There was a problem hiding this comment.
I'd still be interested in landing something like this, and would be happy to review an update. No hurry on my part though.
Sorry, something went wrong.
Fixes: #361
This is a sketch to fix 361. I can see it being desirable to hide the inner recursive parameter by creating an "outer" and "inner" function, but I opted for the most simple approach.