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

Strip whitelist. Move casing warnings to dev tools #6459

Closed
wants to merge 1 commit into from

Conversation

nhunzaker
Copy link
Contributor

I'm sort of hesitant to send out a pull request for this, but a conversation with a co-worker about why React doesn't allow custom attributes (specifically for attribute-based style rules) triggered my curiosity.

This pull request configures DOMPropertyOperations to allow all attributes to pass through. It maintains the whitelist for attributes that need special behavior.

The side-effect of removing properties from the whitelist is that the dev tools no longer warn on casing issues. To get around this, I added some code to the UnknownProperty dev tool for such occurrences.

@jimfb
Copy link
Contributor

jimfb commented Apr 8, 2016

Conceptually, yes, we would like to do something like this. It is being tracked as #140.

Unfortunately, there is some complexity. We would need to warn for a release for any attributes that are currently being dropped (not on the whitelist, thus not rendered to the DOM). The reason is that some people may use the spread operator to pass along all props, and rely on the whitelist to strip out props that aren't applicable to the DOM node.

If you would like to work on adding that warning, we could take that commit for v16, and then re-consider this PR for v17. The warning would need to have line numbers, so it is likely dependent on the work being done in #6398

@jimfb
Copy link
Contributor

jimfb commented Apr 8, 2016

I'm going to close this out for now, since it's not actionable for the next major release (or two). I'll mark it as unsolved so we can come back to it at the appropriate time (do remind us if we forget). But the first step is to start warning whenever we drop an attribute and thus don't render it to the DOM.

This is great work though, much appreciated!

@nhunzaker
Copy link
Contributor Author

Ah, no worries. That totally makes sense. I knew it'd be a long road before shipping something like this.

I've added the warning in #6465.

@nhunzaker
Copy link
Contributor Author

The recent addition of the unsupported property warning (#6800) reminded me of this PR. Is it worth cracking this open again, or is there already work in progress?

No worries either way.

@gaearon
Copy link
Collaborator

gaearon commented Jul 14, 2016

I'm personally not aware of such work right now, and it looks like a good time to me. I might be wrong though but if I were you I'd give it a try now!

@nhunzaker
Copy link
Contributor Author

Cool. Took a quick pass. The rebase was easy but.... many, many tests fail. I think I can have something together Monday or Tuesday.

@nhunzaker
Copy link
Contributor Author

Resubmitted in in #7311

@iRobot98
Copy link

iRobot98 commented Aug 11, 2024

superheroes don't all wear capes

some pass custom attributes to the DOM

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.

5 participants