Patch bad DOM: <img fallbackImage= ...>

[mheiber]

No description provided.

[conorhastings]

@mheiber do you have a reproducable case? this shouldn't be happening in any current version of react.

Something similar to this should go in at some point but I need to think more about whether or not we want to be more aggressive than this

[mheiber]

@conorhastings thanks for following up. I can send a screenshot tomorrow, and this isn't a blocker.

[conorhastings]

@mheiber thanks, no rush, running code would be preferable if possible, you can use https://esnextb.in/ with gists to access es6 imports etc...

[mheiber]

@conorhastings here's an example.

Source code:

Warning message in the console:

These warnings go away when I make the change in this PR.

React dev tools:

[conorhastings]

@mheiber coool cool, what I would expect, doesn't appear to be getting into the actual dom just the virtual dom, I'll think about how to handle this some more and try to come to a conclusion this week if we should just merge this or go in another direction.

[mheiber]

relevant React issue:

facebook/react#6800

I added a comment on that issue asking what the best practice is.

And you're right that the invalid props aren't making it to the actual DOM:

[conorhastings]

@mheiber yup I'm aware of the react issue, there's ongoing debate over the best practice. https://twitter.com/natebirdman/status/759902308993937409

[mheiber]

I'm thinking (based on discussion in the React issue) that deleteing the keys is better than assigning null.

[conorhastings]

@mheiber delete isn't ideal as it's very slow- https://www.smashingmagazine.com/2012/11/writing-fast-memory-efficient-javascript/

[mheiber]

@conorhastings that's why I want to avoid delete, but there doesn't seem to be a good option. On the React issue, people recommended explicitly assigning all properties we want, but <img> actually has a TON of HTML attributes and it would be crazy to assign them all.

The warning they added to React is a mistake, IMO.

[conorhastings]

@mheiber I think the warning makes sense, as in order to properly support web components they need to make all props pass through to the DOM. It follows the long term react pattern of giving warnings for breaking changes before breaking things, and only appears in development and not production which is relatively pragmatic.

Currently the whitelist react is using is also relatively unmaintainable as html properties change.

I'd prefer not to go with delete so I'm going to mull over other other potential options before deciding what to do here.

[conorhastings]

@mheiber I think I'm going to go with this solution https://github.com/socialtables/react-image-fallback/pull/30/files as it will also prevent props that are spread in by consumers of the module and not just ones this component is directly responsible for.

[mheiber]

@conorhastings nice solution. I like the use of the html-attributes module much better than listing out all the possible attributes for an <img> tag.