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
base: master
from

Conversation

Projects
None yet
4 participants
@nhunzaker
Collaborator

nhunzaker commented Apr 8, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Apr 8, 2016

Contributor

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

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

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Apr 8, 2016

Contributor

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!

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

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Apr 8, 2016

Collaborator

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.

Collaborator

nhunzaker commented Apr 8, 2016

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

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Jul 14, 2016

Collaborator

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.

Collaborator

nhunzaker commented Jul 14, 2016

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

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 14, 2016

Member

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!

Member

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

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Jul 15, 2016

Collaborator

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

Collaborator

nhunzaker commented Jul 15, 2016

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

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Jul 20, 2016

Collaborator

Resubmitted in in #7311

Collaborator

nhunzaker commented Jul 20, 2016

Resubmitted in in #7311

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment