Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DOM] Using certain reserved values for the name attribute breaks Scala types #136

Open
raquo opened this issue Feb 2, 2023 · 1 comment

Comments

@raquo
Copy link
Owner

raquo commented Feb 2, 2023

Please read if you are using the name attribute / property of HTML elements.

As you might know, setting the name attribute on an input element, e.g. <input name="method"> elements inside a form will cause the browser to include that input's value in the data that it sends to the server when the parent <form> is submitted.

What's perhaps surprising is that the browser also makes the named element available as a property on the form element, even when the same property name is used for something else already). So in the example above, formElement.method would return a reference to that input element with name="method" instead of returning the actual method property of the form ("get" "post" / etc.), which is normally the case.

This answer describes the issue more clearly: https://stackoverflow.com/questions/7588846/when-to-access-the-attribute-vs-the-property/7590937#7590937

Needless to say, this is disruptive to anyone trying to read the form element's properties in a type safe way. In scala-js-dom, the method property of HTMLFormElement is typed as a String, which honestly is the only sane way to do it. The problem is not limited to that particular method name, and form elements have dozens of properties including inherited ones like id.

In Laminar 15 I need to read properties of elements, so I need to either work around this, or disallow users to use names that are reserved. With a workaround I can only fix Laminar internals to not break, but the types would still be broken (e.g. el.ref.method would potentially return an element instead of the expected string).

I think I should special-case the name attribute in Laminar to make it throw when trying to set a value matching the name of one of the

element's properties. This would be the most correct course of action, however it could also be disruptive, as common names like id, name, width, etc. would become banned, and if you're using them, you can't trivially change these names because they're tied to your backend, or could be part of someone else's API.

So I think I could also provide a nameUnsafe attribute that does not have this check, and lets you set any name, type safety be damned.

Alternatively, I could print an error to the browser console instead of throwing an exception – this will be less disruptive but also less visible, as devs don't always look at the console, and automatic error reporting tools like Sentry default to not reporting non-thrown errors that are merely printed to the console. Also, there's usually lots of spam in the console already.

The question here is whether we should protect users from occasional insanity of the DOM. We already do some small things towards that goal, but nothing as disruptive as this yet.

Either way, this will probably affect you, so please let me know what you think.

@raquo
Copy link
Owner Author

raquo commented Feb 11, 2023

Update: For unrelated reasons, Laminar 15 will no longer need to read current values of form properties from the DOM when updating them, so this issue is no longer relevant to Laminar internals. If nobody has ran into this issue so far, perhaps I will simply document this problem under "special cases" in the main docs.

raquo added a commit that referenced this issue Feb 12, 2023
…etc.

Automatically applying a .distinct filter to DOM updaters is not safe because it does not work as expected when the DOM is updated from multiple sources (e.g. two streams feeding the same attribute, or some external changes affecting the children. The correct way to do this would be to distinct against the current value in the DOM, as opposed to the last emitted value from the stream, but reading the current value from the DOM is actually more expensive than updating it blindly (verified with benchmarks), so this approach makes no sense.

I changed cls from prop to attr because composite keys require reading current value from the DOM, and for
props, that can fail on form elements: #136 (there is no performance
penalty for this according to my benchmarks)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant