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

Remove 'unkown props' from DOM elements; fixes #16 #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ojame
Copy link
Contributor

@ojame ojame commented Jul 13, 2016

This is an interesting problem to 'solve', this was the cleanest and most transparent method I could think of. It touches a bunch of files, though.

Another option was just to const {invalidProps, ...everythingElse} = this.props and then <span ...everythingElse> in a one liner. But it would produce a lot of unused consts and would just get pretty messy.

Let me know your thoughts!

@ojame ojame force-pushed the remove-unknown-props branch from f42ac0c to b4d5794 Compare July 13, 2016 01:56
@mightyaleksey
Copy link
Owner

Hi, thank you for your interest!

Actually, I was going to replace mixin/StyleComponent.js with simple inheritance first and think about this issue after.

As far as I know there are two opportunities to solve it: use the white list and provide only necessary props to the React.createElement or use the black list and omit unnecessary props.
Mostly that issue appear from using custom props styles and styleName and other props that come from the components level (theme, size and etc). And in the simple cases we can manually set the unwanted properties to undefined: https://babeljs.io/repl/#?evaluate=true&lineWrap=false&presets=es2015%2Creact%2Cstage-2&code=function%20Link(props)%20%7B%0A%20%20return%20(%0A%20%20%20%20%3Ca%20%7B...props%7D%20className%3D%7BstyleName(props)%7D%20styleName%3D%7Bundefined%7D%20styles%3D%7Bundefined%7D%2F%3E%0A%20%20)%3B%0A%7D (that avoids necessity for another iteration and object creation).
But I'm not sure what's better to use for the other cases.

I'll try to review yours pr at the weekend and may be I'll come with a more thoughtful answer. Thanks

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.

2 participants