FluentResource entries allocation in FluentBundle is inconsistent

[zbraniecki]

While working on fluent-rs I realized that we currently have a rather undefined around memory allocation in FluentBundle.

add_resource method is intended to take a reference to a FluentResource stored in some ResourceManager (like L10nRegistry), and in result the signature of the FluentBundle might look like this:

struct FluentBundle<'bundle> {
  resources: Vec<&'bundle FluentResource<'bundle>>
}

Unfortunately, at the same time, we have add_messages which accepts a &str and creates a FluentResource out of it and then stores its reference.
The issue here is that we don't have a place where to allocate and retain the FluentResource itself in that scenario.

In JS/python this is messy, but hidden. Rust exposes that inconsistency.

In my current model I'm going for sth like that:

struct ResourceManager {
  strings: HashMap<String, String>,
  resources: HashMap<String, FluentResource>
}

struct FluentBundle<'bundle> {
  resources: Vec<&'bundle FluentResource<'bundle>>
}

so that the resources and their strings are stored on ResourceManager and if two bundles use the same FluentResource they both will get immutable reference to it.

What should we do about add_messages? Is that still useful? If so, would it make sense to require some manager to be available to the bundle such as:

let bundle = FluentBundle::new(resmgr);
bundle.add_messages("key = Value");

or maybe we could consider dropping that and requiring an external resource manager to be used and provide references to the resources?

@stasm - thoughts?

[zbraniecki]

I put more thinking into it, and at this point I'm thinking about it similarly to how I think of Intl memoizer (#218) - we could allow for the constructor to pass a ResourceManager instance that the bundle will use to store strings/asts, or, if no manager is passed, the FluentBundle would initialize its own.

[zbraniecki]

More thinking and prototyping led me eventually in the opposite direction. Instead of trying to allocate a custom ResourceManager in FluentBundle for add_messages I now suggest that we drop add_messages all together and make any customer allocate their own resources if they don't want to use the resource manager.

In result instead of:

bundle.add_messages("hello-world = Hello World!");

we would write:

let res1 = FluentResource::from_string("hello-world = Hello World!");
bundle.add_resource(res1);

I know the change is disruptive, but I believe it's worth considering it to establish a clean API that can be implemented by both JS and Rust alike.

[JohnHeitmann]

I think it's worth removing add_messages if it removes a bunch of complexity from FluentBundle at the cost of an extra simple method call by the caller.

let res1 = FluentResource::from_string("hello-world = Hello World!");
bundle.add_resource(res1);

Is the idea there that ::from_string uses a static store internally so that res1 has a static lifetime? Otherwise I think you run into the problem I just had writing example code that you'd need to make sure res1 outlives bundle. It's ugly in sample code to build resources before the bundle, but maybe that's not a problem in real code.

What if add_resource took a Rc<FluentResource>?

[zbraniecki]

Otherwise I think you run into the problem I just had writing example code that you'd need to make sure res1 outlives bundle.

That's the design we settled on when prototyping with @Manishearth. I know it's not super-pretty, but it's also not intended to be used like that outside of tests and examples.

In the real app, you'll have some ResourceManager that will outlive all bundles, and it'll handle the lifetimes of FluentResource.
In engines such as Amethyst, it'll likely be their manager, but if you need a standalone, fluent crate provides one as you can see in this example: https://github.com/zbraniecki/fluent-rs/blob/copy-free-resolver/fluent/examples/simple.rs#L81

@Manishearth - what's your take on Rc<FluentResource>?

[Manishearth]

I think it's fine, but it does lead to extra allocation and refcounting overhead which is annoying.

Carefully documenting the usage with a resource manager sounds good.

Alternatively: allow it to have both add_resource and add_resource_owned, with the latter taking a FluentResource or Rc<FluentResource>. You can also use Cow.

[JohnHeitmann]

Leaving it alone for now and waiting on real world usage sounds good then. I don't want to push hard for a change based just on a tiny doc example.

[stasm]

@zbraniecki, did you file this issue in fluent.js on purpose? Or should we move it to fluent-rs?

let res1 = FluentResource::from_string("hello-world = Hello World!");
bundle.add_resource(res1);

I'm all for removing add_messages in fluent-rs and doing the above—it's clear, explicit and it reduces the API surface (assuming we'd use impl From<String> for FluentResource and the same for &str rather than a custom from_string method) sorry, I forgot that this method already exists.

[zbraniecki]

@zbraniecki, did you file this issue in fluent.js on purpose?

I filed it here to evaluate removing the method from JS/Python. I'm not sure how tightly we should keep the fluent-bundle API between different implementations and in result part of me is worried about comitting to support add_messages in JS if we know it doesn't fit memory management model in Rust.
Among other reasons to consider the removal - if in the future we'd like to attempt to use fluent-rs-into-wasm behind the scenes for fluent.js this may become a blocker. Not sure how realistic it is tho.

[stasm]

#404 removed the addMessages method from FluentBundle.