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

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: stas, Assigned: stas)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 745866 [details] [diff] [review]
Check if the global is active

Gandalf, does this look right?
Assignee: nobody → stas
Attachment #745866 - Flags: review?(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.
Attachment #745866 - Flags: review?(gandalf) → review+
(Assignee)

Comment 3

6 years ago
Created attachment 746321 [details] [diff] [review]
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.
Attachment #745866 - Attachment is obsolete: true
Attachment #746321 - Flags: review?(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.
Attachment #746321 - Flags: review?(gandalf) → review+
(Assignee)

Comment 5

6 years ago
(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?
land as-is
(Assignee)

Comment 7

6 years ago
Thanks.

Fixed in https://github.com/l20n/l20n.js/commit/55958409218425b1ab7959ef8de98721cfaba067
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

6 years ago
I had to follow up with https://github.com/l20n/l20n.js/commit/6227bcde57c6eacaa723a693d8315a69e02c4776. Sloppiness on my part, sorry.
You need to log in before you can comment on or make changes to this bug.