684721 - GCLI templater and devtools templater need to be in sync

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Joe Walker [:jwalker]]

I don't see any significant functional changes, but the one in GCLI is 'newer' and has been tested against a fuller test suite.

Comment 1

[Joe Walker [:jwalker]]

Differences:
- Allow node to be passed in as either a DOM Element or an ID string
- Be consistent about using var rather than let (this works in a browser)
- Node is abstracted at the top of the file not in the middle
- clone.removeAttribute('foreach'); is done by processSingle rather than processAll
  I can't remember why this is significant off the top of my head, but I think it is
  a bug fix
- Error handling is more descriptive
- Use Function.bind() consistently

This is NOT the templater with the async changes, that can come in a followup bug if needed.

Comment 2

[Joe Walker [:jwalker]]

Attachment: [in-fx-team] upload 1

Now we're as up to date as makes sense, with https://github.com/joewalker/domtemplate/commits/master

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 558483 [details] [diff] [review]
[in-fx-team] upload 1

The changes all make sense and would not cause problems with the style inspector so thumbs up from me. I have logged bug 684958 for integrating the async version because this could help with the style inspector's first load.

Comment 4

[Joe Walker [:jwalker]]

Can you remember who reviewed the v1?

Comment 5

[Joe Walker [:jwalker]]

(In reply to Joe Walker from comment #4)
> Can you remember who reviewed the v1?

Ignore that - I was being lazy, found out that it was Dolske.

Comment 6

[Joe Walker [:jwalker]]

Comment on attachment 558483 [details] [diff] [review]
[in-fx-team] upload 1

Moving this to robcee like bug 684958

Rob: I suggest taking a look at this before bug 684958. The changes here are all cosmetic, but this could ease you into 684958 :)

Comment 7

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 558483 [details] [diff] [review]
[in-fx-team] upload 1

+var Node = Ci.nsIDOMNode;

I think you could use const here instead of var.

+  if (typeof node === 'string') {
+    node = document.getElementById(node);

What if node == null?

I'm generally opposed to having overloaded argument types. It can make for some confusing debugging and forces these types of checks on entry.

Could you not create a processNodeByElementId() method to handle this case or would that break the calling semantics for this?

I'm ok with these changes, specifically the cleanups in your for loops, but sad that this uses var everywhere instead of let.

I would love to see a minimal testsuite for this. I'll accept a follow-up bug for that. It would suck to have multiple consumers of Templater.jsm eventually fail because we weren't testing the templater output.

Comment 8

[Joe Walker [:jwalker]]

(In reply to Rob Campbell [:rc] (robcee) from comment #7)
> Comment on attachment 558483 [details] [diff] [review] [diff] [details] [review]
> upload 1
> 
> +var Node = Ci.nsIDOMNode;
> 
> I think you could use const here instead of var.

Agreed, and we can do that because this line isn't in the web version.
Can I make this change, and a bugfix I found separately when we commit the async patch?


> +  if (typeof node === 'string') {
> +    node = document.getElementById(node);
> 
> What if node == null?
> 
> I'm generally opposed to having overloaded argument types. It can make for
> some confusing debugging and forces these types of checks on entry.
>
> Could you not create a processNodeByElementId() method to handle this case
> or would that break the calling semantics for this?

Warning screed ahead. tl;dr: I think I agree, probably, but could we wait?

I've paced up and down on this. This is called recursively, however I *think* that it's always with an Element rather than a string, so probably no technical objections. Probably.

You're dead right about overloading having dangers. Java list.remove(x) is a great example. With an int it removes that index, with an object it removes the matching item. Hell broke lose when autoboxing was introduced. Element/id is a common idiom though, and there is a difference with the Java example, in that conceptually there is a 1-1 between id and element (ignoring failure cases) so it's like 2 ways of saying the same thing rather than 2 different things with the same name.

Also I'm generally unhappy with the Temaplter API. On one hand it's beautifully simple in being a single function. So, currently it makes no sense for it to be an object, but with async and the possibility to cancel async processing, there could be sense in an object. Perhaps if/when we fix this, a solution to the other will arise?


> I'm ok with these changes, specifically the cleanups in your for loops, but
> sad that this uses var everywhere instead of let.

I too, would much prefer to use let. I take it this is a comiseration comment rather than an actual desire to change, because this works in a browser, so we have no choice...

> I would love to see a minimal testsuite for this. I'll accept a follow-up
> bug for that. It would suck to have multiple consumers of Templater.jsm
> eventually fail because we weren't testing the templater output.

There is a minimal test suite here:
https://github.com/joewalker/domtemplate/blob/master/test/index.html

However I totally agree that this should be usable in Firefox. I'm currently making GCLI's test suite work both on the web and in chrome, and having done that, this should be a simple port.

Thanks.

Comment 9

[Rob Campbell [:rc] (:robcee)]

(In reply to Joe Walker from comment #8)
> (In reply to Rob Campbell [:rc] (robcee) from comment #7)
> > Comment on attachment 558483 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > upload 1
> > 
> > +var Node = Ci.nsIDOMNode;
> > 
> > I think you could use const here instead of var.
> 
> Agreed, and we can do that because this line isn't in the web version.
> Can I make this change, and a bugfix I found separately when we commit the
> async patch?

yes, please do!

> > +  if (typeof node === 'string') {
> > +    node = document.getElementById(node);
> > 
> > What if node == null?
> > 
> > I'm generally opposed to having overloaded argument types. It can make for
> > some confusing debugging and forces these types of checks on entry.
> >
> > Could you not create a processNodeByElementId() method to handle this case
> > or would that break the calling semantics for this?
> 
> Warning screed ahead. tl;dr: I think I agree, probably, but could we wait?

sure!

> I've paced up and down on this. This is called recursively, however I
> *think* that it's always with an Element rather than a string, so probably
> no technical objections. Probably.
> 
> You're dead right about overloading having dangers. Java list.remove(x) is a
> great example. With an int it removes that index, with an object it removes
> the matching item. Hell broke lose when autoboxing was introduced.
> Element/id is a common idiom though, and there is a difference with the Java
> example, in that conceptually there is a 1-1 between id and element
> (ignoring failure cases) so it's like 2 ways of saying the same thing rather
> than 2 different things with the same name.

in theory, yes.

> Also I'm generally unhappy with the Temaplter API. On one hand it's
> beautifully simple in being a single function. So, currently it makes no
> sense for it to be an object, but with async and the possibility to cancel
> async processing, there could be sense in an object. Perhaps if/when we fix
> this, a solution to the other will arise?

Maybe so. My nit about the overloaded argument types was more of a philosophical objection to any real concern about it causing problems. There are some patterns there that make me go, "hmm". Like replacing an argument with another value, for instance.

> > I'm ok with these changes, specifically the cleanups in your for loops, but
> > sad that this uses var everywhere instead of let.
> 
> I too, would much prefer to use let. I take it this is a comiseration
> comment rather than an actual desire to change, because this works in a
> browser, so we have no choice...

yes.

> > I would love to see a minimal testsuite for this. I'll accept a follow-up
> > bug for that. It would suck to have multiple consumers of Templater.jsm
> > eventually fail because we weren't testing the templater output.
> 
> There is a minimal test suite here:
> https://github.com/joewalker/domtemplate/blob/master/test/index.html
> 
> However I totally agree that this should be usable in Firefox. I'm currently
> making GCLI's test suite work both on the web and in chrome, and having done
> that, this should be a simple port.

sweet!

> Thanks.

You're welcome. Reping when you have the bugfix in and I'll land this.

Comment 10

[Joe Walker [:jwalker]]

The var/const thing is part of the patch for bug 684958, so I think this can land.

Comment 11

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 558483 [details] [diff] [review]
[in-fx-team] upload 1

https://hg.mozilla.org/integration/fx-team/rev/1d0257587092

Comment 12

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/1d0257587092