`Node::insert_before` argument should be optional

[koute]

`Node::insert_before` argument should be optional

[Pauan]

You are technically correct, however using parent.insertBefore(child, null) is exactly the same as using parent.appendChild(child), therefore you can do this:

match foo {
  Some(other) => parent.insert_before(child, other),
  None => parent.append_child(child),
}

So I personally think we should not make this change, because the current API is simpler.

[koute]

Hmm... in general I'm torn about this. Yes, it's nice to be inline with the JS APIs, but on the other hand Pauan is right in that having the second argument be an Option seems kind-of unnecessary since you can just use append_child then. (The method is called insert_before, so I think insert_before(node, None) is somewhat confusing as it's not inserting anything before anything.)

We already do this in quite a few other places where we're not taking an Option where technically in the original API the argument is optional.

[Diggsey]

the web decided that None means "insert at the end", but it could just as well mean "insert at the beginning"

No, the only sane choice is to mean insert at the end - just as the multiplication of zero values is one. There's no ambiguity.

[Diggsey]

You could, and this is exactly what I do in my WebSocket PR, however it is more verbose than just:

parent.insert_before(&child, parent.first_child().as_ref());

[Pauan]

@Diggsey Yes, but your proposed change makes every usage of insert_before more verbose when you don't already have an Option to pass in:

parent.insert_before(child, Some(other))

So it's not a strict improvement, it's a trade-off, and I personally don't like the proposed trade-off.

[Pauan]

Rather than changing insert_before (which is a breaking change), why not instead add in a new insert_before_opt method?

parent.insert_before_opt(&child, parent.first_child().as_ref())

[Diggsey]

Using an Option more accurately reflects the underlying API, which stdweb is supposed to be exposing as directly as possible. This is less confusing for people familiar with the javascript APIs and simplifies porting code from javascript to rust.

Adding Some(..) is not particularly verbose, while adding a whole new method carries a maintenance burden.

From an algorithmic point of view, having the "reference node" be optional is better as it avoids adding unnecessary special-case code paths which then need to be tested.

[Diggsey]

Please remember that these APIs are called by programs, not by people: to a person calling append_child in this case is no big deal, for a program it's a whole new rarely-called code path which is an opportunity for bugs.

If you look at any API for accessing linked lists (which is what we have here) they all intentionally avoid the need to special case insertions and other operations like this by appropriate choice of parameter types.

[Pauan]

@Diggsey Your point about "code paths" doesn't exist with my insert_before_opt proposal. And it's not a big maintenance burden, because it's a very simple function that will never need to be changed, because the API is stable.

Also, on a more meta note, the decision about how to handle None has some nuances: the web decided that None means "insert at the end", but it could just as well mean "insert at the beginning". This is a choice that the developer has to make. Hiding that decision doesn't sound very good to me.

So rather than insert_before_opt I would instead be in favor of adding two new methods: insert_first_before and insert_last_before, with the following signatures:

fn insert_first_before< T: INode, U: INode >( &self, new_node: &T, reference_node: Option< &U > );
fn insert_last_before< T: INode, U: INode >( &self, new_node: &T, reference_node: Option< &U > );

The only difference is how they handle None: insert_first_before inserts the node as the first child, insert_last_before inserts the node as the last child.

The programmer can now choose the behavior of None, which is important because there is ambiguity, so they have to choose.