Skip to content
This repository has been archived by the owner on Dec 16, 2021. It is now read-only.

issue with preact-compat and glamor since 3.12 #330

Closed
conorhastings opened this issue Mar 16, 2017 · 7 comments · Fixed by #331
Closed

issue with preact-compat and glamor since 3.12 #330

conorhastings opened this issue Mar 16, 2017 · 7 comments · Fixed by #331

Comments

@conorhastings
Copy link
Contributor

conorhastings commented Mar 16, 2017

Hi! I'm having a super weird issue with preact-compat and glamor. I've narrowed it down to this addition -- https://github.com/developit/preact-compat/blob/master/src/index.js#L92 , if i comment that out the issue goes away.

Basically it seems like somehow glamor is getting a wrong function argument for the second instance of a component (like i said super weird). It only happens if the component has at least one child that is a non string.

I was able to make a relatively small reproducable test case -- https://esnextb.in/?gist=1911224ff4bc0b93a205b10c60b4be83, let me know if you have any ideas thanks!

@conorhastings
Copy link
Contributor Author

Another interesting thing i've found is if the div in has any other properties in addition to the spread it works fine

@developit
Copy link
Member

Ahh - sounds like we need to clone attributes before setting .children! Good catch - your explanation of the second instance thing helped narrow that down.

@conorhastings
Copy link
Contributor Author

If the tree rerenders, all instances will become broken. So it seems like the only one that renders correctly, is the first instnce on the first render.

@conorhastings
Copy link
Contributor Author

@developit ahh nice, I'd be happy to make a PR if you could point me in the rightish direction.

@developit
Copy link
Member

Totally! I think we need to change these lines to clone attributes.

Basically swap this:

let tag = vnode.nodeName,
	attrs = vnode.attributes;

if (!attrs) attrs = vnode.attributes = {};

... out for this:

let tag = vnode.nodeName,
	attrs = vnode.attributes = extend({}, vnode.attributes);

@developit
Copy link
Member

Looks like doing that fixes the issue: https://esnextb.in/?gist=fafa6fad1f8c6b3a9b44a622c1ee9a8b

@developit
Copy link
Member

Released as 3.14.2 💃

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants