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

Assert key is not in Dynamic Attributes #144

Closed

Conversation

jridgewell
Copy link
Contributor

A changing key attribute (but not passed as the key parameter) could cause havoc. Force key to always be static.

This is of particular interest to compile to iDOM build steps, since a runtime attr(name, value) would be impossible to catch:

var attrs = { key: 'key' };

elementOpenStart('div');
for (var name in attrs) {
    attr(name, attrs[name]);
}
elementOpenEnd('div');
elementClose('div');

@sparhami
Copy link
Contributor

sparhami commented Oct 1, 2015

It doesn't seem like we should even allow a static attribute named 'key' or (better) for a pre-rendering flow use some attribute other than 'key'.

@jridgewell
Copy link
Contributor Author

It doesn't seem like we should even allow a static attribute named 'key'

I was planning on using $('[key="key"]') to hook into my own code, though I'm exploring other ideas.

(better) for a pre-rendering flow use some attribute other than 'key'

That might tie in nicely with #50?

@jridgewell
Copy link
Contributor Author

Also worth noting, if the statics and key are properly done, the attribute and the NodeData#key will always be in sync:

var key = 'key';
var statics = ['id', 'id', 'key', key];

function render() {
  statics[3] = key;
  elementVoid('div', key, statics);
}

patch(container, render);
key = 'other';
patch(container, render);

That's because the new key param will force a new div to be created, and it'll have the proper attribute.

A changing key attribute (but not passed as the `key` parameter) could
cause havoc. Force `key` to always be static.

This is of particular interest to compile to iDOM build steps, since a
runtime `attr(name, value)` would be impossible to catch:

```js
var attrs = { key: 'key' };

elementOpenStart('div');
for (var name in attrs) {
    attr(name, attrs[name]);
}
elementOpenEnd('div');
elementClose('div');
```
@jridgewell jridgewell force-pushed the assertNoKeyInDynamicAttributes branch from 2ad11f6 to e8a7632 Compare October 8, 2015 14:54
@jridgewell
Copy link
Contributor Author

Closing due to certain situations where a key cannot be in the statics array:

function render(items) {
  items = items.map((el, i) => {
    return <div id="id" key={i} />;
  });
  return <div>{items}</div>;
}

// - - -

var _statics13 = ["id", "id"];

function render(items) {
  items = items.map(function (el, i) {
    return _jsxWrapper(function () {
      return elementVoid("div", i, _statics13, "key", i);
    });
  });
  elementOpen("div");
    items.forEach((item) => item());
  return elementClose("div");
}

In this case, to properly hoist the statics, I cannot include key.

@jridgewell jridgewell closed this Oct 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants