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

Node reordering issues #115

Closed
yuretz opened this issue Sep 19, 2017 · 24 comments
Closed

Node reordering issues #115

yuretz opened this issue Sep 19, 2017 · 24 comments
Labels

Comments

@yuretz
Copy link
Contributor

yuretz commented Sep 19, 2017

First of all I would like to thank you for your awesome work. This micro-framework/library looks really interesting and very promising indeed.

I was playing with the example provided here and stumbled upon a couple of issues.

The first one is that sometimes, when re-rendering a re-ordered array, the first item is not displayed. I've tried both with data-id attribute in the template and without it, it doesn't help much. What am I doing wrong here?

The second one is just a DOMException being thrown when trying to render the reordered array.

Both issues are illustrated by this fiddle

@yuretz
Copy link
Contributor Author

yuretz commented Sep 19, 2017

Just a small update. Everything renders as expected if the original array is cloned on each update. See this fiddle. However the original example in the documentation suggests that in-place array mutations should be possible too, so I still think this is an issue.

@joshgillies
Copy link

joshgillies commented Sep 19, 2017

hey @yuretz nice bug report. It looks like this bug may have been introduced with #100. Testing your original example against hyperhtml@1.6.7 the list is rendered as expected -
https://jsfiddle.net/L3se6mw4/3/.

@WebReflection
Copy link
Owner

nice bug indeed ... but majinbuu is just fine. There is something weird going on here, the logic is perfectly fine but it misses one call. I will investigate more tomorrow but so far this works, which is reassuring:

const todo = [
  {id: 0, text: 'write documentation'},
  {id: 1, text: 'publish online'}
];
const list = [];
const aura = majinbuu.aura({splice() {
  console.log(arguments);
  return list.splice.apply(list, arguments);
}}, list);

majinbuu(aura, todo);

todo.push({id: 2, text: 'create Code Pen'});
majinbuu(aura, todo);

const strcmp = (a, b) => (a > b ? 1 : (a < b ? -1 : 0));
todo.sort((a, b) => strcmp(a.text, b.text));
majinbuu(aura, todo);

For some reason, the sort splice is invoked once instead of twice ... and not sure why is that.

@WebReflection
Copy link
Owner

WebReflection commented Sep 20, 2017

P.S. and FYI: the reason cloning each object works is that you are creating new wires each time (always different nodes)

@joshgillies
Copy link

Oh and also:

I've tried both with data-id attribute in the template and without it, it doesn't help much.

for the record data-id holds no special meaning in hyperHTML - @WebReflection I wonder if that example should be changed? My worry is that by having <li data-id=${item.id}>...</li> we're confusing existing users of React that there's special meaning there, similar to how you would assign a key to nodes in React which is a hint for optimising the rendering of lists.

WebReflection added a commit that referenced this issue Sep 20, 2017
@WebReflection
Copy link
Owner

that's just an attribute, but here I have bigger problems to deal with ... 🤷‍♂️

@WebReflection
Copy link
Owner

WebReflection commented Sep 20, 2017

@yuretz can you please test this version and tell me if everything looks good to you? Thanks
https://cdn.rawgit.com/WebReflection/hyperHTML/2495c8d46f5ccf63906c729d3f64c8e6abcba7c5/index.js

edit this is the fiddle with this version https://jsfiddle.net/58cnrsq4/

@WebReflection
Copy link
Owner

WebReflection commented Sep 20, 2017

@joshgillies I don't know React much so I don't know what's wrong there. Does react deal in a special way with data-id ? Currently, React is way inferior to hyperHTML and others when it comes to attributes so I'm not sure I should penalize the documentation in favor of React weird cases. hyperHTML just set attributes, only data or events have special meanings but if you think something might really put people off please do file a PR to the documentation site, thank you!

@joshgillies
Copy link

@WebReflection confirmed with @yuretz original example that the latest as of 2495c8d resolves the issue.

Also regarding data-id I guess I was speculating a little, @yuretz if you can confirm whether you thought there was special meaning behind it that would help clear things up. 👍

WebReflection added a commit that referenced this issue Sep 20, 2017
A splice could leave or create duplicates,
but this is not possible with same DOM nodes.

This patch removes from the DOM only nodes that
are not in the list of future nodes that should be added.
@WebReflection
Copy link
Owner

I've also added the sorting test case and it's covered now. Since the fix is super ugly but kinda obvious, I'm going to push this as patch regardless (or I cannot sleep tonight)

@joshgillies
Copy link

@WebReflection adding a test case would be 👌 Nice work!

@WebReflection
Copy link
Owner

v1.8.1 patches this issue. Thanks everyone for tests and checks.

@yuretz
Copy link
Contributor Author

yuretz commented Sep 20, 2017

Wow! That was fast super fast. Thank you for the fix and explanations!

@joshgillies I confirm, that I was indeed confused by data-id attributes and thought they were used in a special way to optimize DOM diffing.

@WebReflection
Copy link
Owner

WebReflection commented Sep 20, 2017

thought they were used in a special way to optimize DOM diffing.

There is no virtual DOM in hyperHTML and the diffing is powered by Levenshtein distance through Majin Buu splice-based engine, which is there to perform the least amount of DOM operations to change from list-1 to list-2 in place.

Attributes in hyperHTML address actual DOM attributes, and nothing else. These are never special, unless these are events, a data attribute to pass directly some info if needed, or attributes natively present in the element prototype such <input value=${content}>.

In hyperHTML the DOM is always optimized, which is also why you don't need, as developer, to hint optimizations, hyperHTML does that for you out of the box.

One question though: is data-id a specific react thing? I might just change the name of that attribute if it has special meanings elsewhere and if it confuses newcomers.

Thanks.

@asapach
Copy link
Contributor

asapach commented Sep 20, 2017

There's a special attribute called key, which is kind of analogous to wire(obj). data-id is in no way special. I think what confused @yuretz was that you're using it in your example: https://viperhtml.js.org/hyperhtml/documentation/#api-1-0

@WebReflection
Copy link
Owner

I am using ... what? I'm just setting a data-id attribute there, not a key attribute

@sourcegr
Copy link
Contributor

I am using ... what? I'm just setting a data-id attribute there, not a key attribute

@asapach said that the use of data-id confused @yuretz, and the data-id is in no way special, even in React.

This is exactly the case. Yuretz was confused and thought that the data-id has some kind of special meaning.

That said, I believe that @joshgillies was also confused about the data-id use on React :) Nothing special about it, as expected. All data-* attributes cannot be used by a framework as special in any way

@WebReflection
Copy link
Owner

WebReflection commented Sep 20, 2017

everyone is confused, including me 😄

I feel like I cannot use attributes in hyperHTML examples or someone will be confused ... React page explicitly describes key as special, not sure why data-id would be consider special in that example, it's just a custom attribute.

Should I name it data-whatever and call it a day?

@asapach
Copy link
Contributor

asapach commented Sep 20, 2017

I'd just remove it to avoid confusion.

@WebReflection
Copy link
Owner

done

@yuretz
Copy link
Contributor Author

yuretz commented Sep 20, 2017

Thank you guys and sorry for confusion I managed to create 😳
I've been probably disoriented by the fact that the framework we are currently using (riot.js) is using data-is attributes in a special way (as a marker for component mounting point) and this caused some weird interference in my mind regarding a possible special use of data-* attributes in hyperHTML, and that was a mistake indeed.
Now, due to @WebReflection and other guys explanations I think I've got a better understanding of how things actually work. 😌 I'll now go play more with it and try not to break it again 😉

@joshgillies
Copy link

LOL This thread it 💯 !

To be clear @papas-source I never said data-id holds special meaning in React 😛 just that's it's (obviously) easy for people coming from other frameworks to expect some type of special attributes React/Preact/Inferno has key and others, Riot with it's special data-* attributes, I could go on but - the final comment and clarification for new comers is (as mentioned by @WebReflection) - hyperHTML holds no special meaning to DOM attributes other than events, and data attribute to pass directly... data!

@sourcegr
Copy link
Contributor

ohh Riot!

I wish there was a library that its syntax was close to Riot's syntax (which is - in my opinion - by far the best component-like friendly syntax) and its internals based on HyperHTML to be fast!

@WebReflection
Copy link
Owner

AFAIK that's already happening ... they were thinking of using hyperHTML

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

No branches or pull requests

5 participants