elm-community / elm-community/html-extra
Consider making `empty` and `attributeIf` SVG safe
- Dominant language
- Elm
- Stars
- 33
- Forks
- 14
- PR merge metrics
- No merged PRs in 30d
Description
I know that this package is called _HTML_ extra – not _SVG_ extra – but unfortunately the Elm compiler allows using for example `attributeIf` on an SVG element, so it’s easy to make mistakes.
Here’s the problem:
`attributeIf` uses `empty` if the condition is False. `empty` is implemented as `Html.Attributes.classList []`. `classList` is implemented in terms of `class`, so `Html.Attributes.classList []` is equivalent to `Html.Attributes.class ""`. In other words, `element.className = ""` will be executed in JS. As documented that’s fine in practice.
Except in SVG. `.className` is a read-only property on SVG elements. Elm adds `'use strict';` to its JS output, and in strict mode trying to write to a read-only property throws an error.
Me and two colleagues recently spent two hours hunting down why Elm’s virtual DOM crashed. No external JS, no browser extensions, just Elm. After commenting out half of the code repeatedly, we found the culprit:
```elm
FeatherIcons.filter
|> Icons.toHtml
[ Html.Attributes.Extra.attributeIf (countFilters > 0) <| fill "currentColor"
]
```
`attributeIf` on an SVG element. Really hard to find since the code above is not even clear that there’s any SVG involved!
Worse, we didn’t notice this during development. We use `parcel` (v1), and it seems to remove the `'use strict';` so no error was thrown. Until we tried to deploy production.
So, what do you think? Is there a way we could try to make these functions less nuclear?
Requirements:
- `empty` should not cause unwanted effects
- `empty` should not cause performance problems
Ideas:
- `empty = Html.Attributes.style "elmEmptyAttribute" ""`. After quick check it looks like it’s hard to tell that `element.style.elmEmptyAttribute = ""` has been called, which is good!
- `empty = Html.Attributes.property "__elmEmptyAttribute" (Json.Encode.string "")`. `element.__elmEmptyAttribute` will be readable afterwards, but does it matter? Can it affect performance?
Needs more investigation, but it could work!
Somewhat related: https://github.com/elm/html/issues/141 and https://github.com/elm/html/issues/232
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.