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

Skip all special props when setting attributes on web components #6276

Merged
merged 1 commit into from
Mar 17, 2016
Merged

Skip all special props when setting attributes on web components #6276

merged 1 commit into from
Mar 17, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Mar 16, 2016

This was introduced as part of #6164 and approved by @spicyj but got reverted in #6243 by accident.
I believe the web component part of it is still relevant (I should’ve split that PR to signal that):

screen shot 2016-03-16 at 21 12 40

This PR just applies the part of #6164 that is still relevant (for web components only).

Test plan: when dangerouslySetInnerHTML is specified, it is no longer copied as an attribute.

screen shot 2016-03-16 at 21 15 25

## Reviewers

@sebmarkbage

@gaearon
Copy link
Collaborator Author

gaearon commented Mar 16, 2016

The only thing I’m not sure about is whether I need keyOf here. This is related to Closure Compiler optimizations that I don’t understand. @spicyj said keyMirror is not necessary but I wonder why keyOf was present then.

@syranide
Copy link
Contributor

This is related to Closure Compiler optimizations that I don’t understand.

@gaearon All you need to understand really is that GCC mangles member properties such as foo.bar into something like a.b, but strings are not and thus foo['bar'] becomes a['bar'] (which is wrong if you're trying to index an object such as {bar: 1}, but correct if it's {'bar': 1}. So if you want to dynamically index a "struct/class" you'll need to use keyOf({bar: null}) because that gets mangled to keyOf({b: null}) and so keyOf simply returns the first key 'b' (which is correct, because after mangling that is the actual name of that member property).

As for keyMirror, it may make more sense if you think of it as keyOf({bar: null}) === keyMirror({foo: null, bar: null}).bar. It's just a lookup-table version of keyOf.

So as far as I can tell you've done it right.

@zpao
Copy link
Member

zpao commented Mar 17, 2016

👍 sorry for not being more careful in the revert.

@gaearon
Copy link
Collaborator Author

gaearon commented Mar 17, 2016

OK this makes sense to me now. Seems like the current approach is the correct one. keyOf was there to avoid introducing a string like 'children' which would be incorrect, but with the current approach we don’t hard code strings at all, which is fine by GCC.

I’ll merge this in because it’s been reviewed before and @zpao just gave a 👍 .

gaearon added a commit that referenced this pull request Mar 17, 2016
Skip all special props when setting attributes on web components
@gaearon gaearon merged commit e04a138 into facebook:master Mar 17, 2016
@gaearon gaearon deleted the fix-custom-components branch March 17, 2016 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants