869382 - Static globals don't have activate(), and throw errors

Status: RESOLVED FIXED

(L20n :: JS Library, defect, P2)

Description

[Staś Małolepszy :stas]

@os doesn't have the activate() method and RetranslationManager::bindGet throws.

Comment 1

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

Attachment: patch

Comment 2

[Staś Małolepszy :stas]

Comment on attachment 758769 [details] [diff] [review]
patch

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

Wouldn't a cleaner approach be to check if activate and deactiatve is defined on the global?

For instance, change this:

    var added = get.globals.filter(function(id) {
      return bound.globals.indexOf(id) === -1;
    });

to this (not tested):

    var added = get.globals.filter(function(id) {
      return bound.globals.indexOf(id) === -1 && this.globals[id].activate;
    }, this);

(do this twice for all 'added' and 'removed' in bindGet)

This way we wouldn't even increase/decrease the counter for static globals.

Your patch work as well, so I'll leave it up to you to figure out which way you like better.

Comment 3

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

Attachment: patch 2

I like your idea better. How does it look for you?

Comment 4

[Staś Małolepszy :stas]

Comment on attachment 758886 [details] [diff] [review]
patch 2

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

Cool, glad you liked it.

One more potential improvement:  in initGlobal, you can return right before _counter[global.id] = 0; if it's a static global.  Is this correct?

Comment 5

[Staś Małolepszy :stas]

Comment on attachment 758886 [details] [diff] [review]
patch 2

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

::: lib/l20n/retranslation.js
@@ +76,4 @@
>          // because we're just adding new entities to the bind
>          bound.callback = get.callback;
>          var added = get.globals.filter(function(id) {
> +          return bound.globals.indexOf(id) === -1 && this.globals[id].activate;

Also, I now see that if we move these tests around, like so

    return this.globals[id].activate && bound.globals.indexOf(id) === -1;

we might avoid one of them if activate is undefined.

@@ +95,5 @@
>            _counter[id]++;
>            this.globals[id].activate();
>          }, this);
>          var removed = bound.globals.filter(function(id) {
> +          return get.globals.indexOf(id) === -1 && this.globals[id].activate;

Should this check for deactivate?

Comment 6

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

https://github.com/l20n/l20n.js/commit/460a6675b245a3e1b4c18b95a2a3d759ccd977df

Comment 7

[Staś Małolepszy :stas]

(In reply to Staś Małolepszy :stas from comment #4)

> One more potential improvement:  in initGlobal, you can return right before
> _counter[global.id] = 0; if it's a static global.  Is this correct?

The landing didn't address this comment.  Any particular reason?

(In reply to Staś Małolepszy :stas from comment #5)
> @@ +95,5 @@
> >            _counter[id]++;
> >            this.globals[id].activate();
> >          }, this);
> >          var removed = bound.globals.filter(function(id) {
> > +          return get.globals.indexOf(id) === -1 && this.globals[id].activate;
> 
> Should this check for deactivate?

I back that out, sorry; I think it's asking for a bug when you define a custom global which only has deactivate.

Comment 8

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

https://github.com/l20n/l20n.js/commit/9c3a7fd5f3bca864cd6b072872003e590f061264

Comment 9

[Staś Małolepszy :stas]

Thanks!

Comment 10

[Staś Małolepszy :stas]

There was a sneaky typo in the patch which I missed originally.  I followed up with https://github.com/l20n/l20n.js/commit/18c36fa3ad39e4508b510b63b6cba24a3d8588d3b

Comment 11

[Staś Małolepszy :stas]

(In reply to Staś Małolepszy :stas from comment #10)
> There was a sneaky typo in the patch which I missed originally.  I followed
> up with
> https://github.com/l20n/l20n.js/commit/
> 18c36fa3ad39e4508b510b63b6cba24a3d8588d3b

The URL should be:

https://github.com/l20n/l20n.js/commit/18c36fa3ad39e4508b510b63b6cba24a3d8588d3

Comment 12

[Staś Małolepszy :stas]

Aaaand another followup, let's hope this time it's the last one:

https://github.com/l20n/l20n.js/commit/078ea32a4f2eeff2de522023a8271b8a1a8864ce

`this` wasn't set to the correct value inside of the filter functions.

I mentioned this in comment 2 but then missed it in my review.  Let's make it a habit to test code before we land it :)