Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overloaded return type seems problematic #6

Closed
domenic opened this issue Feb 20, 2016 · 8 comments
Closed

Overloaded return type seems problematic #6

domenic opened this issue Feb 20, 2016 · 8 comments

Comments

@domenic
Copy link

domenic commented Feb 20, 2016

Moving the discussion from whatwg/dom#150 (comment).

I see a few alternatives:

  • Always return a DocumentFragment
  • Always return a single node (throw if it's not a single node)
    • Optionally, add another helper (htmls??) that always returns an array of nodes
    • Or, make the other helper always return a DocumentFragment

Maybe it's not too bad to have the overloaded return type, since users are always in control of its usage and it should be pretty obvious what the return type is? That is, there's no way to feed user input into a template string, so it should be pretty obvious from source inspection whether you're going to get multiple nodes or one node.

On the other hand, maybe it's not obvious: html

foo

`` creates two nodes, due to the leading whitespace. Even worse,

html`
<p>foo</p>
`;

creates three nodes. Having that throw might be better than having it silently become an array.

@straker
Copy link
Owner

straker commented Feb 20, 2016

I've been thinking about this myself. I like the idea of being able to create sibling nodes when that is your intension, but the fact that you don't always know what will result in an array being returned makes it a bit difficult. I wouldn't mind returning the DocumentFragment, but I don't know if that would be what someone who first uses the API would expect.

Always returning an array isn't a bad idea either, but the interface for appending it do the DOM is a not friendly towards arrays. If we could add a new API similar to appendChild that accepts an array of Nodes, then an array is not a bad option.

@jshcrowthe
Copy link
Collaborator

Returning a DocumentFragment allows developers to leverage the existing DOM API (e.g. appendChild) while also allowing for the creation of sibling nodes. A definite win-win. We just need to document the return type well and, IMO, I don't think there will be any confusion.

In addition to that DocumentFragment has perf benefits when you get dealing with a large number of nodes compared to other DOM manipulation strategies. (See http://jsperf.com/out-of-dom-vs-documentfragment/47 for an example comparison)

@straker
Copy link
Owner

straker commented Feb 20, 2016

Being able to call appendChild once regardless of how many nodes were created is a very nice interface. I'll document that the return type is DocumentFragment and make it happen.

@straker
Copy link
Owner

straker commented Feb 20, 2016

However, would it be an odd interface to have to always find the node you want when you want to attach events to the returned value if we use a DocumentFragment?

var el = html`<button class="btn">click me</button>`;

// adds the event listener to the DocumentFragment and not the button
el.addEventListener('click', function() {   
  console.log('clicked'); 
});
el.click();  // doesn't work

// now I added the event to the button
el.querySelector('button').addEventListener('click', function() { 
  console.log('clicked'); 
});
el.firstChild.click();  // works

document.body.appendChild(el);

@domenic
Copy link
Author

domenic commented Feb 22, 2016

I do agree there is some definite convenience in getting a single node instead of a document fragment. A document fragment is good in many cases but as you point out, it's not great in others.

I guess I kind of tend toward the two-tags solution. node... and `frag`... maybe? node would act like frag but it would throw if the resulting fragment did not contain exactly 1 node, and it would pluck out the main node otherwise.

Maybe that is too confusing though and just asking people to do .firstChild is fine...

@straker
Copy link
Owner

straker commented Feb 23, 2016

Is there a way we could have the best of all worlds? I love that you can call appendChild on a documentFragment regardless of how many childNodes it has, but I still want the convenience of getting one node back to do things to it. I've also had some devs say that they would prefer the array return because [0] is more convenient to work with than firstChild/querySelector, but then it's less convenient to append it to the DOM.

@jshcrowthe suggested that maybe the documentFragment could pass through all event-like actions to the first child node, since a documentFragment never gets added to the page any events attached to it seem pointless. I'm not sure how you would make the distinction of which APIs to proxy through, since maybe the user wanted to call appendChild on the first element and instead it would call it on the documentFragement.

This is definitely a hard problem. I'm torn between a consistent API and the convenience of an overloaded API.

@straker
Copy link
Owner

straker commented Oct 13, 2016

So after a long while letting this just sit in the back of my mind, I'm thinking we should go with two separate functions. One that always returns a single node and one that always returns a doc fragment for multiple sibling nodes. This mimics the behavior of querySelector and querySelectorAll and ensures you always know what you are getting back. I don't see many developers trying to create sibling nodes as much as they would want to create single nodes or single nodes with children.

I'm not sure what we should call the 2nd function for sibling nodes though. htmls probably isn't distinct enough. htmlCollection or htmlList maybe?

@straker
Copy link
Owner

straker commented Jan 21, 2018

Closing as it seems returning a single node for one child and a doc fragment for multiple children hasn't been a problem. Please reopen if further discussion is needed.

@straker straker closed this as completed Jan 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants