Add common PropName, AttrName, ClassName

[milesfrain]

See #29

[thomashoneyman]

Suggested change

	derive newtype instance semigroupClassName :: Semigroup ClassName
	derive newtype instance semigroupClassName :: Semigroup ClassName

[garyb]

I still have a problem with this instance - I can see that argument for wanting to support "class1 class2" as considered to be a class name, as unless we actually make a smart constructor that rejects names with spaces there's no guarantee it's happening, but now there's a problem with this meaning of this instance too IMO:

If I have x :: ClassName and y :: ClassName it seems like the result of that should be ClassName (unwrap x <> " " <> unwrap y). The name ClassName suggests to me that the value is not part of a class name, but it's a whole class name, so by combining them without a space it's constructing a new class name, not combining two class names.

But I can also see why that might be unexpected for some people, which is kinda why I don't like the instance.

[thomashoneyman]

Yes, that's a good point, and it's the reason why in my own applications I have a separate classes function which concatenates with spaces (unlike a standard semigroup instance, which concatenates without spaces (rightly so)). I agree that we shouldn't have a semigroup instance as the law-abiding one is not what you probably want.

[garyb]

It would still be law abiding if it included the space, it's just that I think either instance we choose has the potential to trip people up, depending on how they see the type ("it's just a newtype string wrapper" vs a thing with its own semantics).

[thomashoneyman]

Well, now I can't remember why I thought that was a semigroup law; I must be thinking of the left/right identity laws for monoid.

For what it's worth, we could assert that this is a thing with its own semantics, where ClassName represents a distinct CSS class, and where append includes a separating space. I don't think I've had a conversation with someone using this type in the Halogen world where they saw it as anything else, though that's a small anecdote.

Regardless, if I'm overconfident and the camps are split then it's much better not to have an instance.

[garyb]

It arose here a while back: purescript-halogen/purescript-halogen#512 🙁

[thomashoneyman]

Yea, I say we take it out to avoid the confusion. Thanks for the link!

[hdgarrood]

Either instance is a perfectly fine semigroup; semigroups only need associativity, and the space-adding version’s associativity follows from normal string concatenation being associative. The difference is that if you use the space-adding Semigroup, you can’t have a Monoid instance because there’s no identity element.

[milesfrain]

I say we take it out to avoid the confusion.

Will do. For reference here is where the instance was added for Halogen purescript-halogen/purescript-halogen#451

[thomashoneyman]

I've left a comment on the relevant Halogen issue as well.

[garyb]

I'm not sure calling these "type safe" is exactly right, maybe just "semantic"?

[milesfrain]

Happy to remove it. Just copying what already exists in Halogen https://github.com/purescript-halogen/purescript-halogen/blame/c8b50378b948fcba9e10d75da718fc7dae3a609f/src/Halogen/HTML/Core.purs#L171

[garyb]

I still have a problem with this instance - I can see that argument for wanting to support "class1 class2" as considered to be a class name, as unless we actually make a smart constructor that rejects names with spaces there's no guarantee it's happening, but now there's a problem with this meaning of this instance too IMO:

If I have x :: ClassName and y :: ClassName it seems like the result of that should be ClassName (unwrap x <> " " <> unwrap y). The name ClassName suggests to me that the value is not part of a class name, but it's a whole class name, so by combining them without a space it's constructing a new class name, not combining two class names.

But I can also see why that might be unexpected for some people, which is kinda why I don't like the instance.

[thomashoneyman]

After hearing @garyb's feedback I agree and think his suggestions ought to be applied.

[thomashoneyman]

We can fix the CI error by providing the annotation newtype PropName :: Type -> Type.

[nsaunders]

Out of curiosity, would anyone be able to shed light on why these types were added to web-html instead of web-dom? Some of the seemingly related functions that live in web-dom like className and getAttribute made me wonder about this... Thanks in advance for any thoughts you can share.