fix(core): Do not select plurals for non-top level compilations#362
fix(core): Do not select plurals for non-top level compilations#362dpchamps wants to merge 1 commit intomessageformat:mainfrom
Conversation
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?
| 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?
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.
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' } } }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.
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.
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.