869005 - Values of globals retrieved only with get and getEntity are cached forever

Status: RESOLVED FIXED

(L20n :: JS Library, defect)

Description

[Staś Małolepszy :stas]

If you only use `get` or `getEntity` to retrieve entities with a particular global, the global is never activated, and thus the initial value is cached forever.

Comment 1

[Staś Małolepszy :stas]

Attachment: Check if the global is active

Gandalf, does this look right?

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 745866 [details] [diff] [review]
Check if the global is active

Review of attachment 745866 [details] [diff] [review]:
-----------------------------------------------------------------

add a comment on why not active global can have it's get fired pls.

Comment 3

[Staś Małolepszy :stas]

Attachment: Better patch

I added the comment and refactored the Global class a little bit.  Now, get() has the caching logic, while _get() simply returns the value of the global.

Gandalf, can you take another look at this?

I also filed two follow-ups that I discovered in testing: bug 869372 and bug 869382.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 746321 [details] [diff] [review]
Better patch

Review of attachment 746321 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/l20n/compiler.js
@@ +457,5 @@
> +        } catch (e) {
> +          throw new RuntimeError('Cannot evaluate global ' + name, entry);
> +        }
> +        _references.globals[name] = true;
> +        return [locals, value];

I'm not convinced we should do catch-all wrappings here. If unsafe code throws, it throws and it's a dev problem to solve.

Comment 5

[Staś Małolepszy :stas]

(In reply to Zbigniew Braniecki [:gandalf] from comment #4)
> I'm not convinced we should do catch-all wrappings here. If unsafe code
> throws, it throws and it's a dev problem to solve.

The problem is that it doesn't throw, but instead reject the locale.build(true) promise.  See bug 869372.

OK for landing as-is, or should I change that part?

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

land as-is

Comment 7

[Staś Małolepszy :stas]

Thanks.

Fixed in https://github.com/l20n/l20n.js/commit/55958409218425b1ab7959ef8de98721cfaba067

Comment 8

[Staś Małolepszy :stas]

I had to follow up with https://github.com/l20n/l20n.js/commit/6227bcde57c6eacaa723a693d8315a69e02c4776. Sloppiness on my part, sorry.