FluentBundle.format() should accept a string identifier of a message or a path to an attribute

[stasm]

If #205 lands, we could change MessageContext.format() to accept an identifier of a message and format it directly to string. It would avoid the intermediate step of having to first getMessage the message from the context, which is a bit clunky and is an artifact of how the bindings retrieve translated attributes.

// BEFORE
let message = ctx.getMessage("message-key");
let translation = ctx.format(msg, args);

// AFTER
let translation = ctx.format("message-key", args);

For attributes, we could introduce the following syntax:

// AFTER
let translation = ctx.format("message-key.attr-name", args);

[zbraniecki]

I'd like to suggest a transition period.

First, we would update format to take id or id.attr just like you did in Rust.

We would still preserve the get_message as semi-private way used by FluentDOM only for getting the whole shape all at once (or replace it with format_message which returns already formatted value/attrs all together)

Then we can experiment with using the new format the way you suggested and analyze performance impact. I'm pretty skeptical about it because it just runs more code, but irrelevant, getting format to act the way we expect would be a big step forward and if we keep the get_message or not after that is less significant.

[stasm]

@zbraniecki Would that require moving the following logic into the fluent package?

fluent.js/fluent-dom/src/localization.js

Lines 208 to 227 in 873b409

	function messageFromContext(ctx, errors, id, args) {
	const msg = ctx.getMessage(id);
	const formatted = {
	value: ctx.format(msg, args, errors),
	attributes: null,
	};
	if (msg.attrs) {
	formatted.attributes = [];
	for (const [name, attr] of Object.entries(msg.attrs)) {
	const value = ctx.format(attr, args, errors);
	if (value !== null) {
	formatted.attributes.push({name, value});
	}
	}
	}
	return formatted;
	}

Or do you have something different in mind?

[zbraniecki]

yes, I believe so. It would return a formatted shape Message {value, attributes} in runtime.

[stasm]

I'd prefer to keep this code in fluent-dom and in fluent-react. As you know, I'm not sure it represents the right approach to formatting compound messages. I'd like to keep fluent agnostic and unopinionated. The bindings modules are better suited to host this code given that they are higher up in the ladder of abstraction and can be more opinionated.

Which leads me to conclude that during the transition period, fluent should have three methods:

1. the current getMessage(id: str) -> Message
2. the current format(resolvable: Message|Attribute) -> string?
3. a new formatString(id: str) -> string?

Ideally, I'd prefer the method no. 3 to be called format, but I'm afraid of compatibility issues with fluent-dom and fluent-react. fluent-react currently lists fluent >= 0.8.0 as its peer depenendency. If we change the behavior of format, I think we should first publish a new version of fluent-react (0.8.1) which doesn't allow fluent 0.9.0 as a peer dependency.

[stasm]

All affected Mozilla web projects now use fluent 0.8.0 and fluent-react 0.8.1. In the future, upgrading them to newer versions of both packages should be easier than it used to be. I'm now less concerned about removing methods from FluentBundle.

New proposal:

1. Remove getMessage.
2. Change format to format(id: str) -> string?.
3. Add a new reflection API tentatively called getMessageShape(id: str) -> Shape?.

Shape would be an object providing information about the public interface of the message in question. For the following message:

window =
    .title = Title

the getMessageShape would return a {value: true, attrs: {title: true}} object, or maybe {value: true, attrs: ["title"]}, or even {value: true, attrs: Set {"title"})}.

This API would allow the bindings to query the bundle about the shape of the message before formatting all attributes defined on it. Here's how I'd imagine it could work in pseudo-code:

import {mapBundleSync} from "fluent-sequence";

function getTranslationForElement(elem) {
    let id = elem.getAttribute("data-l10n-id");
    let bestBundle = mapBundleSync(currentBundles, id);
    if (bestBundle == null) {
        return {value: id, attrs: []};
    }
    let info = bestBundle.getMessageShape(id);
    let value = null;
    if (elementAllowsValue(elem)) {
        value = bestBundle.format(id);
    }
    let attrs = {};
    for (let attrName of info.attrs) {
        if (elementAllowsAttribute(elem, attrName)) {
            attrs[attrName] = bestBundle.format(`${id}.${attrName}`);
        }
    }
    return {value, attrs};
}

[stasm]

(I'd still like to measure the performance of iterating over the attributes defined on the message vs. iterating over all attributes allowed for a given element. If the difference is negligible, I might propose to remove getMessageShape in the future, unless other use-cases come up for which it will be useful.)

[zbraniecki]

So, the operations required to retrieve a formatted attributes change from:

1. call formatMessage(id)
2. iterate over msg.attrs and format each of them

to:

1. call getMessageShape(id)
2. iterate over info.attrs and
   2.1) construct an id.attrName string that is passed to format
   2.2) which then deconstructs this id.attrName, retrieves the message behind the id and retrieves it's attrName and formats it.

It seems to me like 2.1 and 2.2 become this quirky operations that don't fit the model, but are there for some higher design principle, rather than operational efficiency.

The idea that for each attribute we have to construct an artificial identifier that instantly gets decomposed into a tuple, and that for each attribute the message has to be retrieved separately inside the format function, seems to me like the wrong abstraction.

Since the model we're discussing in close to 100% cases the attributes in the FTL have the following characteristics:

1. They're allowed
2. They're necessary to localize the element
3. They're all necessary only together

and each scenario where any of the three is not true is an edge or error case, it seems to me like we should be optimizing the number of operations for the predominant scenario, not increasing.

I'm somewhere around -0.9 on the apache voting process scale. It seems to me like a wrong direction - increase in the number of operations to achieve the basic functionality needed for any compound message declarative localization API to work (in particular, DOM) compared to both current model and alternative proposals.

But I can imagine that due to heavy optimizations in JavaScript we may be able to switch to it without noticable performance cost in large scale tests like talos in Gecko. In that case, I would be sad we're doing that, and I would remain with my votum separatum, but wouldn't block here.

[stasm]

It seems to me like 2.1 and 2.2 become this quirky operations that don't fit the model, but are there for some higher design principle, rather than operational efficiency.

One of the reasons behind this design is security. Similar to how the parser counts placeables in a pattern and to how the resolver keeps track of dirty patterns, this design makes the bundle be defensive about how many attributes might be defined on a message before formatting them. A malicious translation from an unverified source could easily slow down formatMessage by an order or two orders of magnitude just by supplying more attributes than expected.

I think this risk is well mitigated for Mozilla where it's safe to assume that most apps will have their translations validated by compare-locales. The low-level API of FluentBundle, however, should be designed to be more robust fashion.

Ultimately, a malicious translation can be just plainly... wrong, which also constitutes an attack vector. Do you think building these guards into the FluentBundle API is worth the slight increase in code complexity?

The idea that for each attribute we have to construct an artificial identifier that instantly gets decomposed into a tuple, and that for each attribute the message has to be retrieved separately inside the format function, seems to me like the wrong abstraction.

Thanks for bringing it up. I'm not too bothered about it, but it might be be a good idea to consider alternatives, like format(id: str, attrname: str?, data: object?). It would make sense to me to try to unify this API across the implementations, but it's not absolutely necessary.

If we go the formatMessage route, I think the base format method should be renamed to formatValue for consistency. Which would be a good thing anyways since the signature would change, too.

[stasm]

Maybe an iterateAttributes(messageId: str, max: number) -> Iterator over [name, formattedValue] would make sense? It would only need to retrieve the message once, it would format the attribute right away, and it would provide a way to cap the number of format operations.