Conflicting approaches to supporting shared properties

[tesujimath]

So, I just noticed the new release, which includes #91. Which addresses a problem which was already addressed by #84.

Maintaining some properties independently of the codegen seems a bit of a hack to me. Isn't the right way to do this to merge #84 and rollback on the alternate approach?

Note that #84 also addresses #88

[megamaddu]

Yes, I agree. Sorry I thought your PR was still in progress. I'll take another look at it soon!

[tesujimath]

@spicydonuts @justinwoo We have shared properties defined in SharedProps in React/Basic/DOM/Internal.purs and also in the codegen. I have reviewed the conflicts, as follows:

property	SharedProps	codegen
allowTransparency	String	Boolean
autoComplete	String	Boolean
autoFocus	String	Boolean
colSpan	Number	Int
contentEditable	String	Boolean
dangerouslySetInnerHTML		{ __html :: String }
draggable	String	Boolean
formNoValidate	String	Boolean
key	String
maxLength	String	Int
minLength	String	Int
noValidate	String	Boolean
onClick	EventHandler
onMouseDown	EventHandler
onMouseEnter	EventHandler
onMouseLeave	EventHandler
onMouseMove	EventHandler
onMouseOut	EventHandler
onMouseOver	EventHandler
onMouseUp	EventHandler
rowSpan	Number	Int
spellCheck	String	Boolean
srcDoc	String	JSX
style	CSS	String
suppressContentEditableWarning		Boolean
tabIndex	String	Int
unselectable	String	Boolean

I propose that SharedProps be dropped, in favour of codegen from react-html-attributes. In most cases the codegen types are better. There are a few issues to be addressed first:

key is missing from react-html-attributes, probably should be added

onClick is listed in react-html-attributes as an attribute of a, but applies more widely, i.e. an attribute of *

onMouse* events are missing from react-html-attributes, and there are many more:
https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers

style should be added to the codegen consts export.types with type CSS.

[megamaddu]

key and ref are both props React allows on any element, but they aren't really props the component will receive. I'd define ReactProps and EventProps in Internal and SharedProps as an alias for mixing together the generated shared type with ReactProps and EventProps.

There are quite a few String props becoming Int props in this generated version. That might be something that works fine in TS/JS that doesn't make sense with PS' type system, where the shared type might define something as an Int and a specific component redefines it as a Number or String. Have you run into that?

[megamaddu]

I wonder if key and ref could be made optional extra props by the element function itself.. I don't think the Union trick would work there though.

[tesujimath]

I think handling key and ref explicitly like you suggest is the way to go. That's easy.

Regarding different types for different elements, that's already addressed by my codegen PR, #84 (comment), and see the typesByElement in codegen/consts.js