Analysis of Elm core issues and patches

[rupert]

Original sheet for gathering issues is here. This is editable if you want to add any more:

Elm Essential Bug Fixes Assessment

I have analyzed what was posted in the original sheet, and started looking through the pull requests. Output of the analysis being prepared here:

Elm Essential Bug Fixes Analysis

[matt.cheely]

I was looking through these and one thing that might be worth adding to the analysis is a “fixable in external package” check. As an example, elm/parser's missing deadEndsToString doesn’t use any private information that I can see, so it would be feasible to implement it in a parser-extras or parser-patch library, and share that through the community. I suspect that could be true for most of these that don’t involve bugs in native code or re-implementation of operators.

[lucamug]

Not sure if I remember correctly, but is not the modBy 0 1 runtime exception an expected behavior? I think this was left as it is for performances reason. If performance is not needed, it is possible to wrap it in a safeModBy that adds a check for the parameter to be non-zero.

safeModBy : Int -> Int -> Maybe Int
safeModBy a b =
    if a == 0 then
        Nothing

    else
        Just (modBy a b)

Maybe unsafeModBy could be a better name for modBy?

It would be interesting to prioritize issues based on several factors. For example: do they have a workaround? How expensive is it? How popular is the package/function with the issue?

[rupert]

@matt.cheely

I was looking through these and one thing that might be worth adding to the analysis is a “fixable in external package” check.

That is sort of the function of the Change to API Behaviour and API Addition checks - changes to a core API might be one way to fix those, but external package might be a better way, especially if the core API is arguably not broken in the first place.

This one is a good example of that; is not allowing 4. to parse as an Int a bug or a feature?

int parser fails on trailing decimal separator

deadEndsToString would be fine in an external package, if it were not for the fact that it already exists in elm/parser and throws a runtime exception. Fixing it in an external package would still leave that broken code in place.

But yes, good idea, I will add a column for Fixable in Non-Core Package.

[rupert]

@lucamug

It would be interesting to prioritize issues based on several factors. For example: do they have a workaround? How expensive is it? How popular is the package/function with the issue?

My intention is to order them by impact of the fix on the rest of the system. Things that can be fixed in isolation are the lowest hanging fruit. So roughly in order of how I arranged the checks in the spreadsheet:

1. Pure Fix (Optimisation or Tail Recursion or Docs)
2. Runtime Exception
3. Change to API Behaviour
4. API Addition (Backwards Compatible)
5. API Change (Breaking)
6. Change to Language Semantics

But yes, you could play around with the order based on a number of other factors too.

[malaire]

@rupert

This one is a good example of that; is not allowing 4. to parse as an Int a bug or a feature?

You could ask if that is bug or feature, but what about 4d parsing as Int but 4e not? It’s quite hard to argue that not allowing e is a feature.

int parser fails on trailing "e"

[rupert]

@malaire

You could ask if that is bug or feature, but what about 4d parsing as Int but 4e not? It’s quite hard to argue that not allowing e is a feature.

Lets add it to the sheet, looks fixable.

[miniBill]

I think line 32 and 14 are basically the same thing?

Excellent work with the analysis!

[rupert]

Analyzed all of the open PRs on elm/core now:

Elm Essential Bug Fixes Analysis

[rupert]

Added all the open PRs from elm/virtual-dom

Feel like I should add 2 more check box columns to the sheet.

1. For claimed performance improvements, as these should really be backed up with some benchmarks, in some cases its not really clear if a PR for a claimed perf improvement is really going to be a perf improvement.

2. For PRs that have raised some controversy in the discussion. Sometimes Evan might have commented to explain why he does not think something is a good idea, for example. Just to help flag up things that might seem straightforward but really need to be given some careful consideration.

Elm Essential Bug Fixes Analysis

[Latty]

On CSS custom properties, you can find the argument for inclusion here: CSS Custom Properties

This is a vital feature for compatibility with web components and other modern web features, this issue is only going to get more and more important and painful as time goes on.

[rupert]

The fix seems harmless to me, in that it only allows new behaviour to be possible without breaking the old. In that sense, it does seem like the sort of thing we can and should fix.

I marked it as controversial simply because Evan weighed in with the opinion that its not how he envisages CSS being done in Elm (you would write a function in Elm to generate the CSS you want). When it comes to patching stuff in the core independantly of Evan, these are the kinds of issues that need careful consideration so I am glad you brought it up.

Do you have an opinion on how the fix might affect performance? Fix is to change:

domNodeStyle[key] = styles[key];

to

domNodeStyle.setProperty(key, styles[key]);

I know that mdgriffith/elm-ui has put a lot of thought into making its CSS very fast, and I am wary of upsetting that with this change. There is a UI benchmark somewhere that compares elm-ui, elm-css and a few other ways of styling isn’t there? Would be worth checking against that.

I’m not enough of a CSS expert to know if .setProperty is going to be slower.

[lydell]

Regarding .setProperty, there’s some unfortunate backwards compatibility to consider too: Allow CSS custom properties by using style.setProperty() by harrysarson · Pull Request #127 · elm/virtual-dom · GitHub

[Latty]

It may well be that exact implementation isn’t the best solution, if it makes more sense to have a special setCustomProperty that can only be used for that job to separate the implementation or some other alternate solution, that’s reasonable.

My focus is that the feature needs to be surfaced otherwise Elm is going to end up making any modern webcomponent unusable, and blocking access to other modern features (e.g: Houdini), I’m not advocating for any specific solution to surfacing it.

[rupert]

The spreadsheet has now been completed to cover all open Pull Requests on all core packages and the compiler.

Still happy to add any more issues that anyone wants to bring to my attention?

Other than that, that is as much as I am going to add to it for now. Next step will be to order things by level of impact to reveal what can be fixed without breaking anything versus changes that have deeper impacts that are not backwards compatible, or alter the language semantics. Then start a new thread with a summary of the spreadsheet around that topic.

Elm Essential Bug Fixes Analysis

[andre-dietrich]

Maybe this can be added as an additional bug/fix … As I found, elm-apps crash when screen-readers are used:

Elm Application Crashes ... using a ScreenReader

the same topic was also discussed here:

Elm app not compatible with ChromeVox screen reader

When I try to run my Elm app with ChromeVox, I see a continual stream of “Uncaught TypeError: Cannot read property ‘childNodes’ of undefined”. What I’m guessing is I’m hitting a case similar to what is described here: i.e. it’s caused by Elm taking over the body. Do I have any resolution to make my app compatible with screen readers?

Additionally, the same error occurs, when googletranslate is used. All systems change the dom slightly, however this patch seems to work and solves the problem …

GitHub - jinjor/elm-break-dom

[rupert]

@andre-dietrich

Maybe this can be added as an additional bug/fix … As I found, elm-apps crash when screen-readers are used:

Added it. I think the issue was already covered under the item “Browser extensions messing with the VDOM”, but no harm in having variations on it.

I marked this one as “API Change (Breaking)” as maybe we might need to change the Browser.application function type, so it accepts some further parameters that tell it where to put the Elm DOM. That isn’t the only way this could be fixed though, there are more backwards compatible ways it could be tackled.

[andre-dietrich]

Hi, maybe not the function-type, the patch only adds some checks to the function itself. It does not look very complex from my perspective:

jinjor/elm-break-dom/blob/master/patch/VirtualDom.patch

[lydell]

@rupert

I marked this one as “API Change (Breaking)” as maybe we might need to change the Browser.application function type, so it accepts some further parameters that tell it where to put the Elm DOM. That isn’t the only way this could be fixed though, there are more backwards compatible ways it could be tackled.

FYI: Just rendering inside a <div> instead of directly inside <body> is not enough to fix the issue. Something in the spirit of the patch linked to above is needed.

[rupert]

@lydell

FYI: Just rendering inside a <div> instead of directly inside <body> is not enough to fix the issue. Something in the spirit of the patch linked to above is needed.

I see. I didn’t actually see the patch since I didn’t go so far as digging into the jinjor/elm-break-dom project to look for it. I wonder why this patch was never submitted as a PR to elm/virtual-dom? Perhaps the author thought there was no point as it would not be merged?

Only comment I could find from Evan on the issue is:

https://github.com/elm/html/issues/44#issuecomment-233145854

In the past, people (I believe including @rtfeldman) expressed frustration with having that additional named div in there, so I took it out. So I’m surprised by this recommendation. I’m not sure what to do given the history of this.

(the extra div approach screws up printer layouts apparently)

I will change the status of this one to “Runtime Exception” - seems like the patch could be a straightforward bug fix and a really helpful one too.

==============

Found some more from Evan:

Fullscreen Elm App in 0.19 (childNode issue) Reopened

For some additional context: I think adding things at the end of the <body> should be fine as is. I think it is possible to add some logic in virtual-dom to make it safe to add things at the beginning of <body> as well. Not 100% certain, but there is hope. So if the extensions are all in these two categories, we may be able to just have things work out. If the extensions are adding things in the middle of stuff, I’m not sure why they’d be doing it in the <body> any more than they’d be doing …

Possibly that post got jinjor started on compiling a list of problems and coming up with a fix. We just never completed the process and got the fix out…