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

Fix potential inherited prototype chain issue #1091

Closed
wants to merge 5 commits into from
Closed

Conversation

oliviertassinari
Copy link
Contributor

@oliviertassinari oliviertassinari commented Apr 18, 2019

What would you like to add/fix?

name equals $$typeof in this fail.

Capture d’écran 2019-04-18 à 12 11 27

@HenriBeck
Copy link
Member

But why would a vnode get used as a plugin? 🤔

@HenriBeck
Copy link
Member

About the slow commit: I think we should run most checks in the CI and not allow merging without a passing build (though this is currently because external contributors build will always fail and browser stack often timeouts)

@oliviertassinari
Copy link
Contributor Author

oliviertassinari commented Apr 18, 2019

@HenriBeck This is an excellent question. I haven't looked at why. I have assumed that the JSS should only take the plugin own properties and ignore the prototype chain.

@developit
Copy link

This is a result of importing preact-compat against preact@10. The two are incompatible, but instead of throwing an error, preact-compat will actually decorate Object.prototype with a bunch of stuff.

(the reason is because preact-compat tries to install properties on the VNode class used by preact 8.x, and it does so by doing: Object.defineProperties(h('a').constructor, ...) - however, in Preact 10+, h('a').constructor === Object)

@oliviertassinari
Copy link
Contributor Author

I confirm that it works perfectly with preact/compat. I'm closing the pull request. It's too time-consuming to make it pass :).

@oliviertassinari
Copy link
Contributor Author

@developit Thank you!

@developit
Copy link

Thank you for bringing this back to our attention! We've just published an update to preact-compat that should make it warn when installing alongside Preact 10+.

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.

None yet

3 participants