fix: whitelisting and adding observedAttributes in render #55
Conversation
mtscrb
force-pushed
the
fix-attrs-render
branch
from
cf2d4f8 to
5079093
Compare
| @@ -27,6 +27,13 @@ function syncEvent(node, eventName, newEventHandler) { | |||
| } | |||
|
|
|||
| export default function (CustomElement, opts) { | |||
| function whitelistAttrs(props) { | |||
There was a problem hiding this comment.
I'm unsure if this is the best name for it, but I don't want to 🚲 shed too much on it.
There was a problem hiding this comment.
Yes I think it's wrong too. Maybe customElementAttrs or getObservedAttrs ? I'm not so good with names 😆
| if (name.indexOf('on') === 0 && name[2] === name[2].toUpperCase()) { | ||
| syncEvent(node, name.substring(2), props[name]); | ||
| } else { | ||
| node[name] = props[name]; | ||
| } | ||
| }); | ||
| } | ||
| onRef(node) { | ||
| if (node != null) { | ||
| this.applyProps(node, this.props); |
There was a problem hiding this comment.
nit: indentation (eslint should have picked this up, though)
| if (name.indexOf('on') === 0 && name[2] === name[2].toUpperCase()) { | ||
| syncEvent(node, name.substring(2), props[name]); | ||
| } else { | ||
| node[name] = props[name]; | ||
| } | ||
| }); | ||
| } | ||
| onRef(node) { | ||
| if (node != null) { |
There was a problem hiding this comment.
Can node ever be anything but an element or falsy?
There was a problem hiding this comment.
I guess this is just being defensive. ref callback will always have the element right?
There was a problem hiding this comment.
Oh wait, I was conflating this with Skate. React does call ref with null (but I think it's always null). You could probably just use if (node). If you want to be 100% safe, if (node instanceof HTMLElement) would work, but I think that's overkill and truthy / falsy checks here work just fine.
| componentDidMount() { | ||
| this.componentWillReceiveProps(this.props); | ||
| constructor(props) { | ||
| super(props); |
There was a problem hiding this comment.
This seems unrelated to this particular fix?
|
Closing as I think this doesn't solve a concrete purpose. Happy to revisit if I'm wrong. |
fixes #54