Runtime exception: passing "" to Html.node

[rtfeldman]

To reproduce, do this in Try Elm:

import Html

main = Html.node "" [] []

Result is the runtime exception:

InvalidCharacterError: Failed to execute 'createElement' on 'Document': The tag name provided ('') is not a valid name.

Related: https://github.com/elm-lang/html/issues/46 - same behavior for passing "" to Attribute.

[process-bot]

Thanks for the issue! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

[evancz]

What should happen instead? Is it worth it to add a check on every createElement that ever happens under any circumstances?

[rtfeldman]

Definitely not, but I could see making the dangerous one internal-use only, and having a public-facing customNode : String -> Result String (List Attribute a -> List Html a -> Html a)

That way end users could do this:

sidebar : List Attribute a -> List Html a -> Html a
sidebar =
    case Html.customNode "sidebar" of
        Ok fn ->
            fn

        Err err ->
            Debug.crash "Invalid node"

In this world there's an inconsequential bit of page load overhead, but explosions are gonna happen on startup (like they do with Regex.regex) rather than only when the corner case code gets run. The internal helpers remain just as fast as they are today.

This is more work for people who want to make custom nodes, but to be fair, it is for sure an advanced feature.

[evancz]

How do you know if a node is good on startup? Would we need to maintain a list of all valid tags? If so, the maintenance burden of that seems worse than the original problem.

[rtfeldman]

Hm, good point...it's easy enough to check for "" but there are probably others.

Maybe the best resolution here is to add a big warning saying "if you pass an invalid string here, this can crash" or something to that effect?

[douglascorrea]

According with DOM Spec the createElement method can raise DOMException when invalid characters are passed to it. So, this is the only case createElement can raise an error and could be the answer to identify valid node names.

From HTML Syntax spec, we can see that valid names are only 0-9, a-z and A-Z.

But I also checked that createElement raises the exception with tagNames starting with 0-9.

So, checking the conditions above should be enough to use signature suggested by @rtfeldman

[rtfeldman]

Yeah I wouldn't trust that there aren't more cases like that lurking out there, especially given that you immediately found one that wasn't in the spec. Also, cross-browser. 😞

[douglascorrea]

Yeah, it is possible to have more cases out there, but I think your suggestion is the "best effort" to avoid RunTimeException, and the checks above could be the starting point to use it. If we found other possible, non-documented, runtime exception from createElement then we improve our check.

I just think that, as you said, node function is used by advanced users, they can handle Result as output and it will make it less error-prone.

[evancz]

Why do all this work when the suggestion is to have it Debug.crash anyway? We make the type fancier, it's a pain to set up, and in the end it can still crash if this list is wrong... So to me it sounds like we are choosing between these two things:

1. I maintain some arbitrary list of "bad" things that is probably wrong. When the bad list finds something, we want people to Debug.crash out. When the bad list is wrong, an exception is thrown. Either way, it's my fault that things go wrong because I'm maintaining a list.
2. Keep it the same. elm-lang/html is a thin wrapper on top of HTML. If HTML goes wrong, what do you expect? HTML would go wrong. Using node directly is an advanced feature.

In both cases bad things crash. In one case everyone does less work and the results are ultimately the same.

I'll add that the only reason we even know about this is because of #46. As far as we know, this case has never come up in practice. It certainly has not been reported. So this either (1) never happens or (2) when it does happen, no one is upset about it. That is not the kind of motivation we usually have for breaking changes.

[rtfeldman]

Yeah, that all makes sense to me.

I feel like it could be worth adding a warning to the docs to the effect of "Be careful! This can throw a runtime exception if you give it an invalid node name" but that's about it.

[douglascorrea]

I had to agree with you @evancz, and you bring a very important point: the change will be a breaking change.