Remove container default

[mindplay-dk]

Just an opinion based on my personal learning experience with this library.

I'd like to suggest removing the default value for container in the patch function.

I found it quite surprising that there was any default at all - but since the default is document.body, your first experience, if you're following the example, will be that of your own document getting wiped out, which was pretty surprising to me.

Also, when looking at the example in the README for the first time, my first thought was, "if both of the arguments are virtual DOM nodes, how does it figure out which real DOM element to updated?"

Of course, you learn these things quickly if you start poking through the source-code, but most people will likely jump in without reading documentation, or copy/pasting the example - making this argument non-optional might remove any confusion about how a target element is established and/or what happened to your existing body-content.

Defaults don't always curb the learning curve - sometimes they increase it.

[jorgebucaran]

@mindplay-dk I found it quite surprising that there was any default at all.

I prefer libraries that try to do something useful by default, but I can see where you're coming from here. Let's do it then.

Feel free to send me a PR with the necessary changes, including test, types and doc updates please. 👍🙏

[mindplay-dk]

Cool, I will try to find time for that :-)

Since this is a breaking change, how would you feel about swapping the argument order, placing the element argument first? So it reads patch(element, ...) - since literally what you're patching is the DOM element and not the VNodes.

[mindplay-dk]

Alright, removed the default argument for starters.

I noticed a couple of minor things along the way:

1. The argument names in patch and patchElement are inconsistent - it's target in one and container in the other. The most descriptive would probably be parent? It's of course also a container, but it's perhaps more accurately the parent container. Or is there a reason you named them differently?

2. The documentation says "Patch returns the patched HTML element" - I was a bit confused by this at first, it sounds like it's returning the same element I gave as the argument. Perhaps more accurately, "Patch returns the patched child element"? (it's also not always an HTML argument - could be SVG, as indicated by the Element rather than HTMLElement return-type in the .d.ts, which I believe is right?)

Note that I also updated the description of patch near the end of the README - it was partially type-hinted, only for the first argument; I added the other two types, though I wonder if these type-hints ought to appear after // comments? The colons are perhaps confusing, as it's not valid JS, and not really even valid TS. (?)

[jorgebucaran]

@mindplay-dk Good questions.

1. There is a reason, and as you may guess it has to do with byte saving. I wouldn't mind changing the order if that would make picodom easier to use! 👍

2. Let's change the docs then. The current state of the docs is embarrassing, and that's my fault.

[mindplay-dk]

Let's leave the PR open then, I'll do both of these before the merge, yeah?

[jorgebucaran]

You've encouraged me to improve picodom in ways that will make it depart a bit from hyperapp's internal VDOM, and that's fine! There a few other internal (possibly non-breaking) changes that I have in mind, that will make the code more —1— readable and —2— more "functional" (e.g., how we deal with lifecycle callback flushing).

Picodom's goal to remain simple and stay within the vicinity of 1 KB has not changed, but there is still a lot of room for activities! 💪😉

[mindplay-dk]

I think that's why this project is different - I can see it being directly useful without ever needing a bunch more features. Honestly this may be the first front end project I've ever been truly enthusiastic about ;-)

[mindplay-dk]

Just a thought, but you might want to merge this to a 2.0.0 branch initially, in case you think of any other breaking changes you'd like to get in before the next release. (any idea what makes the @next constraint work for some packages in npm? so we could start testing and upgrading to the next version more easily.)

[jorgebucaran]

@mindplay-dk I think I can just publish any tag, it doesn't have to next, so that anyone can start playing with the new stuff.

git tag 2.0.0-beta
git push --tags
npm publish picodom@2.0.0-beta
...
npm i picodom@2.0.0-beta

I need to double check, but something like the above should probably work.

[jorgebucaran]

@mindplay-dk The argument names in patch and patchElement are inconsistent - it's target in one and container in the other.

We can make patch and patchElement signatures consistent too with:

patch(oldNode, newNode, parent)

So, why put the parent first again?

The most descriptive would probably be parent?

I think container tells me more as a Picodom user, whereas parent tells me more when reading the source code. It used to be called parent, actually, but got renamed to container to be in sync with Hyperapp.

Now, from a "sales" standpoint, I think container sounds better, but I don't mind renaming it to parent. And the point about being in sync with Hyperapp doesn't really matter.

[mindplay-dk]

why put the parent first again?

Because the parent element is the one that actually gets patched - you're not patching the old or new VNodes, they only provide the context for patching the actual element.

It's a matter of preference, I guess, but I like following this formula:

function verb(subject, object...)

So I'd prefer updating the argument order of patchElement() also, for the same reason - I find patchElement(what_element, ...) to be more intuitive than patchElement(..., what_element, ...)

Now, from a "sales" standpoint, I think container sounds better, but I don't mind renaming it to parent.

I guess I prefer parent because that element is the parent of the elements that get patched. It's also a container, but almost all elements are containers of something.

The most important thing for me is consistent terminolgy within the codebase and documentation though, so if you prefer container, let me know, I'll change it back and make that the consistent term :-)

[jorgebucaran]

Sure that more than one opinion on what the best formula is, there can be, I am.

[mindplay-dk]

Should I update patchElement() to match patch() then before we merge, or?

[jorgebucaran]

No, I like your changes and just merged them. #41