Skip to content
This repository has been archived by the owner on Sep 7, 2020. It is now read-only.

Fix automatic keys and Elements with a single child #73

Closed
wants to merge 5 commits into from
Closed

Fix automatic keys and Elements with a single child #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9a6f2df
Generate appended IDs recursively
Sep 8, 2016
1096958
Add automatic unique keys for connectFauxDOM mixin
Sep 8, 2016
8a5f883
Remove automatic IDs from core mixin
kitten Sep 12, 2016
120f8f1
Add non-nested solution to automatic keys
kitten Sep 12, 2016
5531e62
Remove key getter and setter for property in constructor
kitten Sep 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 24 additions & 15 deletions lib/Element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,16 @@ var isUndefined = require('./utils/isUndefined')
var assign = require('./utils/assign')
var mapValues = require('./utils/mapValues')
var styleCamelCase = require('./utils/styleCamelCase')
var uniqueID = require('./utils/uniqueID')

function Element (nodeName, parentNode) {
this.nodeName = nodeName
this.parentNode = parentNode
this.childNodes = []
this.eventListeners = {}
this.text = ''
this.key = uniqueID()

var self = this
var props = this.props = {
ref: function (component) {
Expand Down Expand Up @@ -245,20 +248,15 @@ Element.prototype.getBoundingClientRect = function () {
return this.component.getBoundingClientRect()
}

Element.prototype.toReact = function (index) {
index = index || 0
Element.prototype.toReact = function (id) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what's stopping us using my refactored "single node" aware recursion and leaving the IDs as they are? All functionality remains the same, single elements don't require keys and the user can set a different key if they need to place two children side by side. I fail to see why the global UUID is required to fix the original issue, could we not keep the stateless key approach and just document how to change the top level keys?

var props = assign({}, this.props)
props.style = assign({}, props.style)

var originalElement = this

function uniqueKey () {
return 'faux-dom-' + index
if (isUndefined(props.key) && id) {
Copy link

@FunkMonkey FunkMonkey Sep 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@philpl I think we're also missing a check for this._key and need to set props.key to this._key if necessary. Otherwise this._key has never been used. Right?

[edit] well. we still use key and thus this._key later, but this._key will not be used if we call toReact directly and set a key before...

var el = ReactFauxDOM.createElement('div');
el.key = 'foobar';
el.toReact();

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really not sure about _ prefixed methods + getters/setters. I think
there's a much simpler way. I'll reply with a PR or something soon.

On 13 September 2016 at 19:30, FunkMonkey notifications@github.com wrote:

In lib/Element.js
#73 (comment):

var props = assign({}, this.props)
props.style = assign({}, props.style)

  • props.key = isUndefined(props.key) ? idPrefix + '-' + index : props.key
  • if (isUndefined(props.key) && id) {

@philpl https://github.com/philpl I think we're also missing a check
for this._key and need to set it to props.key if necessary. Otherwise
this._key has never been used. Right?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/Olical/react-faux-dom/pull/73/files/8a5f8837612a0ee97ba73a56073f2fe837c4a925..120f8f10bff6468fec5f92c41e3b7c765c2cd407#r78617435,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AATPXav-Kt_k3RuT4IXJwkgCoxssSJk3ks5qpuvggaJpZM4J33UN
.

props.key = id
}

if (isUndefined(props.key)) {
props.key = uniqueKey()
}
var originalElement = this

delete props.style.setProperty
delete props.style.getProperty
Expand All @@ -279,13 +277,24 @@ Element.prototype.toReact = function (index) {
}
}))

return React.createElement(this.nodeName, props, this.text || this.children.map(function (el, i) {
if (el instanceof Element) {
return el.toReact(i)
} else {
var children

if (this.text) {
children = this.text
} else if (this.children.length === 1) {
var el = this.children[0]
children = el instanceof Element ? el.toReact() : el
} else if (this.children.length > 1) {
children = this.children.map(function (el) {
if (el instanceof Element) {
return el.toReact(el.key)
}

return el
}
}))
})
}

return React.createElement(this.nodeName, props, children)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where I would like to introduce my little refactor, simplifying and moving into it's own function: https://github.com/Olical/react-faux-dom/pull/74/files#diff-52cea43ae897a1705ec51162aed25f63R276

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do. Are you sure you want to add the function after its occurrence?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's hoisted regardless. I would move it to another file and require it but it becomes a cyclic dependency :(

You could add it above if you want, I don't mind.

}

Object.defineProperties(Element.prototype, {
Expand Down
9 changes: 9 additions & 0 deletions lib/utils/uniqueID.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
var index = 0

// This is of course not using UUIDs, but should be totally sufficient
// for our use case
function uniqueID () {
return 'faux-dom-' + (index++)
}
Copy link
Owner

@Olical Olical Sep 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I don't like is that this requires you to build your element and keep it outside of your render method. It implies that you can not generate your element on the fly on every render (which I how I intended the library to be used). I mean, you can do that but you'll have new ids every time so it'll be slow.

I just don't like the statefulness of it, personally. I build everything stateless unless state is required because it leads to cleaner and less buggy code that's easier to read in six months time.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, you can do that but you'll have new ids every time so it'll be slow.

Are we sure about that? Is it going to be slower? Anyway. Maybe there is a more deterministic method of creating the keys instead of using a global counter. Though I don't think there is a stateless way, considering my next point ...

I just don't like the statefulness of it, personally.

I do understand this. In your use-case the statefulness is not really necessary. In other user-cases, where faux DOM nodes are moved around, there is not way around it...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I build everything stateless unless state is required because it leads to cleaner and less buggy code

State is required in this case. I'm totally for minimising state, but in this case you're just handing the task of managing that state to your users.

The point is, this is not perfect when we're creating elements on the fly, but that's when the users should help out and provide keys themselves -- which then doesn't make a difference to what you're proposing. React's heuristic 1-1 diffing of lists without keys is also really sub optimal.

But when we're creating elements beforehand, the user doesn't have to do anything. This is a huge step forward.

I'd focus on how we can either try to remove our keys when we're creating elements on the fly, or how we can output nice warnings in that case, or both.


module.exports = uniqueID
24 changes: 15 additions & 9 deletions test/react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,23 @@ test('nested text', function (t) {

test('auto default keys', function (t) {
var el = mk()
// 0
el.append('p')
el.append('p').attr('key', 'testing')
// 1
var sub = el.append('p').attr('key', 'testing')
// 1.0
sub.append('p')
// 2
el.append('p').attr('foo', 'bar')

var tree = el.node().toReact()
var tree = el.node().toReact('test')

t.plan(5)
t.equal(tree.key, 'faux-dom-0')
t.equal(tree.props.children[0].key, 'faux-dom-0')
t.plan(6)
t.equal(tree.key, 'test')
t.notEqual(tree.props.children[0].key, undefined)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is now pretty vague, all of the notEqual ones are. It could be equal to the entire works of Shakespeare and still pass 😢

t.equal(tree.props.children[1].key, 'testing')
t.equal(tree.props.children[2].key, 'faux-dom-2')
t.equal(tree.props.children[1].props.children._key, undefined)
t.notEqual(tree.props.children[2].key, undefined)
t.equal(tree.props.children[2].props.foo, 'bar')
})

Expand All @@ -51,7 +57,7 @@ test('pre-built React elements are rendered into the tree', function (t) {
var tree = el.toReact()

t.plan(1)
t.equal(tree.props.children[0].props.foo, 'bar')
t.equal(tree.props.children.props.foo, 'bar')
})

test('React elements customize data-* attributes are rendered into the tree', function (t) {
Expand All @@ -65,7 +71,7 @@ test('React elements customize data-* attributes are rendered into the tree', fu
var tree = el.toReact()

t.plan(1)
t.equal(tree.props.children[0].props['data-foo'], 'bar')
t.equal(tree.props.children.props['data-foo'], 'bar')
})

test('React elements aria-* attributes are rendered into the tree', function (t) {
Expand All @@ -79,7 +85,7 @@ test('React elements aria-* attributes are rendered into the tree', function (t)
var tree = el.toReact()

t.plan(1)
t.equal(tree.props.children[0].props['aria-hidden'], 'true')
t.equal(tree.props.children.props['aria-hidden'], 'true')
})

test('toReact does not mutate the state', function (t) {
Expand Down