-
Notifications
You must be signed in to change notification settings - Fork 30
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
Re-render markup when components prop changes #29
Conversation
src/index.js
Outdated
let p = this.props; | ||
shouldComponentUpdate({ wrap, type, markup, components }) { | ||
const p = this.props; | ||
if (Object.keys(components).join()!==Object.keys(p.components).join()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I'd suggested this implementation previously, but I think we might be able to swap it and save a few bytes+ticks. Also Object.keys(undefined)
throws, which seems like it would make the component error out if components={}
weren't passed.
if (components) {
// we have a list of components, but there wasn't one before:
if (!p.components) return true;
for (let i in components) {
// one of the components changed:
if (p.components[i]!==components[i]) return true;
}
for (let i in p.components) {
// one of the components was removed:
if (!(i in components)) return true;
}
}
else if (p.components) {
// we used to have a list of components, but don't anymore:
return true;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Sorry it took me so long to reply but I've done the change now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that the lib got bumped to preact X (👍) but now my new tests don't run (preact-render-spy
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should work with two render()
calls - the tests run in a browser environment, so rendering is okay:
const XFoo = () => (<div class="x-foo">hello foo</div>);
const XBar = () => (<div class="x-bar">hello bar</div>);
render(
<Markup type="html" markup='<x-component />' components={{ XComponent: XFoo }} />,
scratch
);
expect(scratch.textContent).to.equal('hello foo');
render(
<Markup type="html" markup='<x-component />' components={{ XComponent: XBar }} />,
scratch
);
expect(scratch.textContent).to.equal('hello bar');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(admittedly, this is less nice than the preact-render-spy version. I really want to make things work together here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, real life happened and I had completely forgotten about this until now. I've updated the test with your suggestion. Thanks for the help 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh I'm with you there, thanks for jumping back in though!
It's weird that Github isn't showing the easy "update" button - I fixed the test runner issue on master. |
I rebased with master anyway, that should get Travis running |
Hey, can we get this merged? It would really help in my current project. |
This PR looks like it could / should be merged? :) I would just change this comment:
...with:
(to more accurately describe the possible cases) |
Fix for issue #25: change the
shouldComponentUpdate
function to allow re-rendering when thecomponents
prop changes.