Skip to content
This repository has been archived by the owner on May 27, 2018. It is now read-only.

why not return a fragment instead of a node list ? #1

Open
kapouer opened this issue Apr 18, 2017 · 14 comments
Open

why not return a fragment instead of a node list ? #1

kapouer opened this issue Apr 18, 2017 · 14 comments

Comments

@kapouer
Copy link

kapouer commented Apr 18, 2017

Handling fragments is simpler, because it works with native methods:
document.body.appendChild(Inst.list('<p></p><p></p>'))

@loilo
Copy link
Member

loilo commented Apr 18, 2017

Ouch. Shame on me, I didn't even know DocumentFragment was a thing. 👍

That would certainly be the better approach and even make the .list() method completely obsolete since a DocumentFragment could be returned just in all cases.

@kapouer
Copy link
Author

kapouer commented Apr 28, 2017

It's cool to be able to return a single node when it's a single node that is expected.
I'm in favor of an implicit api: return a node or a fragment depending on what's happening.

@loilo
Copy link
Member

loilo commented Apr 28, 2017

Yep, I'd support that idea. In fact—as opposed to what I thought—that wouldn't even be a breaking change.

@loilo
Copy link
Member

loilo commented Apr 30, 2017

I'm working on this right now and would like to hear your opinion: Do you see any downsides to returning a DocumentFragment no matter what?

It would definitely simplify the API compared to determining return types by number of elements.

@kapouer
Copy link
Author

kapouer commented Apr 30, 2017

Well, i've been using dom string templates for a project and most of the time, it's cool to be able to directly have the top node under the fingers. However, frag.firstChild is not so terrible...

@kapouer
Copy link
Author

kapouer commented Apr 30, 2017

Unless frag.firstChild is some text node :(. However, you might not want whitespace trimming. Or you might want it - my opinion is "don't change things unless necessary".

@loilo
Copy link
Member

loilo commented Apr 30, 2017

Yeah, point taken. I've just read through some issues over at straker/html-tagged-template and I currently favor Domenic's suggestion of having two methods, since I'm just not too sure about overloading the return type of a function.

So basically that would mean having the domify function that'd always return a DocumentFragment and something like domify.node which would assume one element, return a Node then and throw otherwise.

However, that would in fact be a breaking change. I'm not sure much I'd value not releasing a new major version since almost nobody uses this package at the moment. (That issue generally could easily be fixed by defeating my terrible habit of releasing everything as 1.0.0 right away...)

So, to keep the current API, things could also be done the other way around: The current behaviour could be kept as the single-node option while adding a new alternative to domify.list (e.g. domify.fragment) which then would return a DocumentFragment.

Thoughts?

@kapouer
Copy link
Author

kapouer commented Apr 30, 2017

Fair enough, though a bit verbose, but that can be fixed client-side.

@kapouer
Copy link
Author

kapouer commented Apr 30, 2017

Also i think the .list method, with its monkey-patching, is ill-defined and should be deprecated.

@loilo
Copy link
Member

loilo commented Apr 30, 2017

Totally agreed.

Verbosity should be fixable with ease, a const {fragment} = require('domify-template-strings') should suffice.

Any thoughts on the method name though? fragment feels a little long to me but I couldn't come up with anything more terse so far.

@kapouer
Copy link
Author

kapouer commented Apr 30, 2017

frag ?
nodes ?
all ?

@loilo
Copy link
Member

loilo commented Apr 30, 2017

I've read that over at Domenic's suggestion. It feels a little weird to me since I associate it with shooter games (even if I never actually played those), but I guess since I'm apparently the only one with that feeling I guess it's fine. :)

@kapouer
Copy link
Author

kapouer commented Apr 30, 2017

I've updated the comment, please check the other ones. all is my favorite.

@loilo
Copy link
Member

loilo commented Apr 30, 2017

I like it as well. So, all it is.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants