added: Support to disable named elements on loops

[imagentleman]

Content

When you add an id or name to an element, it gets automatically added to the context for easy access (http://riotjs.com/guide/#named-elements).

But, that feature turns out to be a big performance hit on loops.

Here is an example of a loop of 500 elements with ids:
https://plnkr.co/edit/FPOPNLVXoXaIr9LnaRVj?p=preview

It takes about 400ms to 2s to render in my machine. : S

The same exact html without ids on the elements of the loop:
https://plnkr.co/edit/GdWFFekcbvBvW2bdCdSq?p=preview

Renders in about 10-20ms.

This pr adds support to disabling that automatic bounding of named elements via a 'no-named-elements' attribute:

<div id={ id } each={ items } no-named-elements></div>

We could argue that we just shouldn't use ids on loops, but there might be scenarios where we just have to (e.g. third party libraries that depend on ids).

We could also workaround this issue by preventing the UI from having so many elements, but working with hundreds of nodes doesn't sound like an unreasonable front end use case, and again, there might be an scenario when this isn't possible (e.g. a search widget where the backend api can't provide paginated results and we just filter on the UI).

Code

1. Have you added test(s) for your patch? If not, why not?
   Yes.
2. Can you provide an example of your patch in use?

Example with the new riot+compiler.js, that uses the attribute to disable the named elements:
http://plnkr.co/edit/lV2c26h1HW0q9CK7Ssvt?p=preview

Example with the new riot+compiler.js, that doesn't use the attribute to disable the named elements:
https://plnkr.co/edit/E4w0PJECkT1zBvtVXN9b?p=preview

1. Is this a breaking change?
   No.

[rsbondi]

This looks good, I like the idea of it being optional. I almost lean toward holding off for v3 and make the named elements an option rather than the default, curious about what others think.

[GianlucaGuarini]

@imagentleman thanks for your patch but here I get slowly the impression we are patching the issue instead of fixing it. I will refactor the loop rendering in riot 3.0.0 and the current version without my new implementation runs already a bit faster https://plnkr.co/edit/FPOPNLVXoXaIr9LnaRVj?p=preview
I will keep this pull request open being aware that the named DOM elements can be really an issue in loops

[wintercounter]

To be honest, I never liked the automatic access when using name or id attributes. What really hurts is name when I need to use inputs for example in the markup. When a checkbox name is an array I'll end up having a this['checkbox[]'] property. Also within a component the name is usually dynamic and I cannot refer it in my code in case I also need to use ID for a specific reason, and have to make workarounds. I understand the simplicity behind the feature but my taste would prefer a way to make it accessible ONLY when I want to. I don't have any suggestions for this, and I know you wouldn't want to introduce a new attribute for this, I'm just saying my thoughts :)

[rvion]

I also do not really like the feature as it is, even if it is sometimes quite handy. I would prefer named elements binding to be optional by default. How about a something like <input bind=varname> ?
(on my projects, I often have large loops, this issue bit me in the past)

[deckchairlabs]

I would suggest something like how the React world do it, using ref="my-unique-name"

[cognitom]

I like the idea using ref="somthing". See also #1185 and #715.
@TrajEdy how do they resolve the multiple reference with ref in React?

[GianlucaGuarini]

@cognitom could you please try a pull request on riot@next? for this behavior I also like the idea and it will speed up also the loop performances.
cc: @rsbondi @aMarCruz @rogueg @tipiirai

[cognitom]

@GianlucaGuarini sure, I'll try that 😉

[wintercounter]

A list should be kept about breaking changes like this one, so update will
be smoother to 3.0.

On Fri, Jun 24, 2016 at 4:51 PM, Tsutomu Kawamura notifications@github.com
wrote:

@GianlucaGuarini https://github.com/GianlucaGuarini sure, I'll try that
😉

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1806 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AA60wCA8jcqw1DuPPm3SkqOzX8QbLH6Zks5qO-8NgaJpZM4IkG0b
.

[rogueg]

I also like the idea of changing it to ref. I wonder if this would let us clear up #1783 as well.

[GianlucaGuarini]

@rogueg @cognitom cool I like the idea.. this will be a big breaking change but it's worth to do it and it will solve many issues

[deckchairlabs]

@cognitom https://facebook.github.io/react/docs/more-about-refs.html#the-ref-callback-attribute

I suppose from React it would be more like ref="{ checkboxes }" where checkboxes would be an array defined on the instance? So when mounted it would automatically add a reference to itself.

<my-checkboxes>
  <my-checkbox ref="{ checkboxes }" />
  <script>
  this.checkboxes = []

  // ...
  // Later on 
  this.submitHandler = () => {
    _.each(this.checkboxes, function(checkbox) {
      // checkbox is tag instance of <my-checkbox />
    })
  }
  </script>
</my-checkboxes>

Perhaps? Where the reference would be to the Tag, and not the DOM element?

[cognitom]

@TrajEdy it makes sense. Gotcha. Thank you!

[GianlucaGuarini]

we are going to merge the ref attributes instead of names or ids #1917 I am closing this pull request and I want to thank @imagentleman for his great feedback

[imagentleman]

Glad i could help start the discussion. The ref approach looks definitely better. : )