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

Warn if ChildNode is used as modifier multiple times. #184

Open
raquo opened this issue Jan 22, 2025 · 3 comments
Open

Warn if ChildNode is used as modifier multiple times. #184

raquo opened this issue Jan 22, 2025 · 3 comments

Comments

@raquo
Copy link
Owner

raquo commented Jan 22, 2025

Discussed in #183

Originally posted by ivan-klass January 22, 2025
Recently, I've encountered an issue with some elements being not displayed where expected.

Here's a simplified and minimized example to demonstrate my case:

See https://scastie.scala-lang.org/8dB25QZfRtW0ZuamN4x88w

import com.raquo.laminar.api.L.{*, given}
import org.scalajs.dom
import scala.scalajs.js

val brick = span("[BRICK]", styleAttr := "background-color: orange")
val tenBricks = div(Seq.fill(10)(brick)*) // won't contain 10 children in practice

// A helper to render Laminar elements in Scastie
def renderApp(app: dom.Element, content: => Element) =
  js.timers.setTimeout(0) { render(app, content) }
  app

renderApp(dom.document.createElement("div"), tenBricks) // renders only one brick, no warning

The root case traces back to a lack of referential transparency: an expression of the node declaration can't be assigned to a val and reused. That's because there is an allocation side-effect, which is probably fine to have as Laminar don't pretend to be FP framework. With mixed feelings, I've completely fixed my particular problem by simply changing val definition to def.

If I understand correctly, using an element as a modifier is equivalent to setting a parent, and the most common case is when there were no parent before.

Given the above, I think maybe it would make sense to have a warning when element is used as modifier while the parent is already set. In case when changing the parent was intentional, maybe it's better to have an explicit mechanism (e.g. method, function, event) for that, rather than using modifier syntax?

@raquo
Copy link
Owner Author

raquo commented Jan 22, 2025

@ivan-klass Yep, our elements are real, not virtual. That's the expected tradeoff.

But yeah, I think we can add such warnings. I think, perhaps they could be warnings-only, so they would not throw an error, just inform the user of the issue via the browser console. Similar to the duplicate key warnings that we already have. Actually, the warnings you propose could theoretically replace the duplicate key warnings, except they probably wouldn't provide enough details. But, they're cheaper.

Anyway. This issue you brought up is related to moving elements, and we have a bunch of tasks in this general area shaping up in #163 – I will need to find a good place to integrate these warnings, and to provide a way to opt-out of them, where moving the element is intentional – perhaps using the same movable modifier contemplated in #163. I'm not yet sure how it would work technically – we'll probably need to set an isMovable flag on any element that is supposed to be movable (and that would among other things suppress your suggested warnings for this element), but it does seem that it's quite possible to provide these warnings reliably and cheaply.

@ivan-klass
Copy link

ivan-klass commented Jan 23, 2025

@raquo thanks. Just as an idea, maybe add something like

// explicitly move child from current owner to another parent
ChildCommand.TransferToNewParent(elementExpectedToBecurrentChild, newParent, position = ???)
// or
// explicitly "adopt" an element, replacing its parent to current owner
ChildCommand.BecomeNewParentFor(childOfAnotherElement, position = ???)

?

@raquo
Copy link
Owner Author

raquo commented Jan 23, 2025

@ivan-klass You can basically already do this with Laminar's DomApi low level interface. But we can't support element moving only in the low level interface, all the standard Laminar features need to work with it as well. So, we need to create a modifier like movable that we can add to an element to declare the intent to move it in the future, e.g. div(movable, "hello world").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants