Removed Default Keys (bugfix from last pull request)

[rileyatodd]

Note: same as last pull request that I closed just got a little overzealous last time and deleted one line I shouldn't have which caused some errors

Problem

The setting of default keys was causing problems. It often gave two elements the same key in which case React only renders one of them. I initially tried to solve this by adding sequence numbers to the keys but this resulted in React thinking it was a new component every time it rendered because the key got incremented every time. Eventually I realized that if no key was provided, React automatically kept track of the components by incrementing its data-reactid attribute.

By trying to provide default keys, jsnox was breaking the default React behavior. Trying to avoid the "Each child in an array should have a unique key prop" warning by assigning default keys actually led to explicit duplicate keys which is a much bigger problem because the components with duplicate keys that were explicitly assigned don't get displayed.

Solution

To remedy the problems I was encountering I removed all key setting code from jsnox. A simple fix with the added benefit of making the package smaller and more focused.

Conclusion

I propose that jsnox should not try to assign keys at all. If a developer is getting warnings about unique keys they should assign keys intentionally as shown here:

http://facebook.github.io/react/docs/multiple-components.html#dynamic-children

Default key assignment is not transparent and causes errors that many people might not understand unless they look at the source code for jsnox. Lets keep it simple and return to the default React behavior regarding keys.

[af]

Thanks, I will take a closer look at this as soon as I can. Which version
of react are you using? I don't think the data-reactid attribute was
preventing these issues on 0.12-0.13 but I haven't tested 0.14 yet.
On Jul 19, 2015 9:23 AM, "Riley A Todd" [email protected] wrote:

Note: same as last pull request that I closed just got a little
overzealous last time and deleted one line I shouldn't have which caused
some errors
Problem

The setting of default keys was causing problems. It often gave two
elements the same key in which case React only renders one of them. I
initially tried to solve this by adding sequence numbers to the keys but
this resulted in React thinking it was a new component every time it
rendered because the key got incremented every time. Eventually I realized
that if no key was provided, React automatically kept track of the
components by incrementing its data-reactid attribute.

By trying to provide default keys, jsnox was breaking the default React
behavior. Trying to avoid the "Each child in an array should have a unique
key prop" warning by assigning default keys actually led to explicit
duplicate keys which is a much bigger problem because the components with
duplicate keys that were explicitly assigned don't get displayed.
Solution

To remedy the problems I was encountering I removed all key setting code
from jsnox. A simple fix with the added benefit of making the package
smaller and more focused.
Conclusion

I propose that jsnox should not try to assign keys at all. If a developer
is getting warnings about unique keys they should assign keys intentionally
as shown here:

http://facebook.github.io/react/docs/multiple-components.html#dynamic-children

Default key assignment is not transparent and causes errors that many
people might not understand unless they look at the source code for jsnox.

Lets keep it simple and return to the default React behavior regarding keys.

You can view, comment on, or merge this pull request online at:

#9
Commit Summary

• Added sequence numbers to generated keys so that they will always be
  unique
• removed a semicolon so that style would match that of original author
• Removed everything regarding setting keys.
• Bug fix

File Changes

• M jsnox.js https://github.com/af/JSnoX/pull/9/files#diff-0 (16)

Patch Links:

• https://github.com/af/JSnoX/pull/9.patch
• https://github.com/af/JSnoX/pull/9.diff

—
Reply to this email directly or view it on GitHub
#9.

[rileyatodd]

I'm using 0.13.3.
The main issue is that the warning about each child having a unique key is one that really should be heard by the developer (especially those new to React), and doesn't seem to cause any actual issues in simple components. Having sibling components with explicitly set duplicate keys on the other hand produces not only a more severe warning but also side effects which include only one of the components being rendered.

For example:

If you try going through the main React tutorial (where you build a comment box) and only change jsx to jsnox, only one comment will be rendered because no keys were set in the actual code for the comment box and they all get explicitly set to 'customElement' by JSnoX. React will throw a warning such as

Warning: flattenChildren(...): Encountered two children with the same key. Child keys must be unique; when two children share a key, only the first child will be used.

By removing the explicit key setting parts of JSnoX, React just throws an an 'every child should have a unique key' warning but React still displays all of them correctly.

In essence by trying to avoid the small warning that children should have unique keys set on them by their parent component, we have caused a much bigger problem of explicitly set duplicate keys, which React doesn't handle as gracefully compared to if we had not provided keys at all.

[af]

So after playing around with this in one of my side projects that uses jsnox, I'm getting warnings that I didn't have before, where key auto-generation was actually quite nice to have. Primarily cases like this (sorry for the contrived example):

render() {
    // Each <li> has a different className, and thus, a nice automatic key:
    return d('ul', items.forEach(itemId => d(`li.${itemId}`, itemId)))
}

At the same time time, your point is quite valid about the automatic keys being unnecessary and problematic sometimes. How about having the key generation being an option (disabled by default) that is activated by the spec string? Maybe a ^ suffix or something like that. I think that could give us the best of both worlds. Thoughts?

[iofjuupasli]

I think you shouldn't mess class with key. Sometimes they can match. But even in this case it's better to separate them as they have different purpose.

React team have decided to just use key for arrays, without automatic (automagic) keys.

My opinion is that jsnox should be simple wrapper for React.createElement.

After using jsonx for some time, I've decided to create my own custom wrapper.

Before I'm used this simple shortcut:

var h = React.createElement;

And it works very well. The only thing that was annoying is providing className in props each time.

For cases when I need dynamic class (or dynamic key) it seems better to use props with cx.

👍 for merge

[rileyatodd]

I think leaving the key generation in as an option on the spec string (disabled by default) is a great, elegant compromise here. It let's people that are used to the current key generation continue using it with just minor modifications, and removes the possible confusion for people just picking it up! I can work on adding that later today and update the pull request.

[rileyatodd]

I decided to add the option on the initial construction of the JSnoX function. Several reasons here, including the fact that adding it as an option on the specString prevents that functionality from being used on custom components. Additionally, if you don't want the old key generating behavior all you have to do is make one small change at the very top of your file instead of hunting down all your specStrings and changing them individually. Lastly, there is no additional regex this way so it is more performant. I think it's ready to be merged now but let me know what you think.

[af]

Hmm, not sure about setting that as a construction-time option. Personally I will want to both use and disable autokeying at different points within the same file, so having it in the spec string is attractive. As for custom components, the autokeying is very basic and probably not that useful for them. We might as well get rid of it TBH

[rileyatodd]

Okay well if we're gonna get rid of it for the custom component then it makes more sense to have it on the spec string. I'll put it in as a specString option. Should I leave the construction-time option in for people that wan't to keep the key generation on all specString without hunting down all of them?

[rileyatodd]

Got rid of auto key generation entirely for custom components, added ability to enable it in the specString with a trailing ^ and left in ability to enable it for all specString declared components at construction-time to ease migration.

[af]

Thanks! Haven't had a chance to run it yet but here are some things that come to mind:

• I think the 2nd option to jsnox() should be an options object for readability and future flexibility. Usage could look like var d = jsnox(React, { autokeyAll: true })
• Any alternate suggestions for the ^ suffix? Just want to explore options before committing to it. + or *? Or maybe I'm just overthinking this.

[rileyatodd]

I think ^ is a solid choice, I can add another option in the new construction time options object for changing it to something else? That might be a bit much though. Maybe ^^ as a double would almost never occur naturally. Probably won't get around to making these changes until next week though.

[af]

No problem, I should be able to tackle it this weekend. ^ is probably ok, just wanted to consider any possible alternatives.

[af]

Hey, I rebased your changes and pushed a new branch: https://github.com/af/JSnoX/tree/autokey-pr

One notable change is that I removed the 2nd argument to jsnox() entirely (the one to enable autokeying for all elements). Two reasons for this:

• It caused some problems when you had 2 jsnox clients (one with, one without autokey). Since the clients share the same parsed specString cache, and the same specString (eg 'div.foo^') gives a different result between clients, you can get incorrect cached results.
• You've convinced me that autokeying everything is really a bad idea :)

Let me know what you think!

[rileyatodd]

The new branch looks great! Can't think of any more improvements to make =) It seems like the cleanest version to me.

[af]

Great, thanks for the PR and discussion! I'll try and cut a v2.0.0 in the next day or so with these changes.