Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an IntlMemoizer as an argument that can be shared between MessageContexts #254

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zbraniecki
Copy link
Collaborator

This will allow us to implement https://bugzilla.mozilla.org/show_bug.cgi?id=1475356 and utilize a shared intl memoizer between contexts.

@zbraniecki zbraniecki requested a review from stasm July 13, 2018 00:39
}

return cache[id];
return this._intlMemoizer.get(ctor, this.locales, opts);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this method completely and use ctx._intlMemoizer directly in the resolver (in types.js).

@@ -0,0 +1,17 @@
export default class IntlMemoizer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove some boilerplate by subclassing WeakMap here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, should we export IntlMemoizer in index.js?

const cache = this._intls.get(ctor) || new Map();
const id = {locales, ...opts};

if (!cache.has(id)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks rather Pythonic to me :) Sadly, it won't work in JS. id is a new object literal every time this code runs. cache.has(id) will always be false.

@Demivan
Copy link
Contributor

Demivan commented Oct 25, 2021

@eemeli This, probably, can be closed now. #504 implements this functionality.
Edit: And #218

@eemeli
Copy link
Member

eemeli commented Oct 25, 2021

@Demivan Good point, this does look obsolete. @zbraniecki, would you agree?

@zbraniecki
Copy link
Collaborator Author

umm, this is actually concerning. With #504 we just made it mandatory for bundles to share memoizer. My mental model is that we should be careful about it as there may be side effect of crating opportunity for one bundle to affect another by tainting the shared memoizer and its cache.

In my vision we would by default isolate memoizers per bundle or registry, and allow users to volunteerily provide a shared memoizer to be used.

This way the API is explicit about "this bundle shares memoizer with other bundles" and noticeable to the consumer in any security review.

This would also make it easy to align with multithreaded implementations of Fluent (like fluent-rs) where the option to pass a memoizer wouldn't be available so the default behavior would be the same (isolated memoizers), while the optional "pass a shared memoizer" wouldn't which is reasonable.

@eemeli
Copy link
Member

eemeli commented Oct 26, 2021

@zbraniecki While I get your point about there being a possible security concern in the now-merged shared memoizer allowing for an Intl object to be a communication pathway between bundles, I have two follow-on questions:

  • As we currently consider the memoiser as an internal implementation detail, are you ok with Add per locale Intl memoizer #504 having been merged, or should it be rolled back?
  • For runtimes that need to take this into account as a security concern or otherwise, should the memoiser sharing be controllable per-bundle or per-runtime? As in, if we want to prevent some sharing should we then prevent all sharing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants