Skip to content

Commit

Permalink
Fix: Was binding insertFn to initial, not current, hooks
Browse files Browse the repository at this point in the history
  • Loading branch information
raquo committed Dec 4, 2023
1 parent de9da36 commit b59f019
Show file tree
Hide file tree
Showing 11 changed files with 33 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package com.raquo.laminar.inserters
import com.raquo.airstream.core.Observable
import com.raquo.laminar.modifiers.RenderableNode
import com.raquo.laminar.nodes.{ChildNode, ParentNode}
import org.scalajs.dom

import scala.scalajs.js

Expand All @@ -11,11 +12,11 @@ object ChildInserter {
def apply[Component] (
childSource: Observable[Component],
renderable: RenderableNode[Component],
hooks: js.UndefOr[InserterHooks]
initialHooks: js.UndefOr[InserterHooks]
): DynamicInserter = {
new DynamicInserter(
preferStrictMode = true,
insertFn = (ctx, owner) => {
insertFn = (ctx, owner, hooks) => {
// Reset sentinel node on binding too, don't wait for events
if (!ctx.strictMode) {
ctx.forceSetStrictMode()
Expand All @@ -27,7 +28,7 @@ object ChildInserter {
maybeLastSeenChild = newChildNode
}(owner)
},
hooks = hooks
hooks = initialHooks
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ object ChildTextInserter {
): DynamicInserter = {
new DynamicInserter(
preferStrictMode = false,
insertFn = (ctx, owner) => {
insertFn = (ctx, owner, _) => {
var maybeTextNode: js.UndefOr[TextNode] = js.undefined
textSource.foreach { newValue =>
maybeTextNode.fold {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ object ChildrenCommandInserter {
def apply[Component] (
commands: EventStream[CollectionCommand[Component]],
renderableNode: RenderableNode[Component],
hooks: js.UndefOr[InserterHooks]
initialHooks: js.UndefOr[InserterHooks]
): DynamicInserter = {
new DynamicInserter(
preferStrictMode = true,
insertFn = (ctx, owner) => {
insertFn = (ctx, owner, hooks) => {
commands.foreach { command =>
val nodeCountDiff = updateList(
command,
Expand All @@ -41,7 +41,7 @@ object ChildrenCommandInserter {
ctx.extraNodeCount += nodeCountDiff
}(owner)
},
hooks = hooks
hooks = initialHooks
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ object ChildrenInserter {
def apply[Component](
childrenSource: Observable[immutable.Seq[Component]],
renderableNode: RenderableNode[Component],
hooks: js.UndefOr[InserterHooks]
initialHooks: js.UndefOr[InserterHooks]
): DynamicInserter = {
new DynamicInserter(
preferStrictMode = true,
insertFn = (ctx, owner) => {
insertFn = (ctx, owner, hooks) => {
// Reset sentinel node on binding too, don't wait for events
if (!ctx.strictMode) {
ctx.forceSetStrictMode()
Expand All @@ -51,7 +51,7 @@ object ChildrenInserter {
// }
}(owner)
},
hooks = hooks
hooks = initialHooks
)
}

Expand Down
7 changes: 4 additions & 3 deletions src/main/scala/com/raquo/laminar/inserters/Inserter.scala
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ trait StaticInserter extends Inserter {
class DynamicInserter(
initialContext: Option[InsertContext] = None,
preferStrictMode: Boolean,
insertFn: (InsertContext, Owner) => Subscription,
insertFn: (InsertContext, Owner, js.UndefOr[InserterHooks]) => Subscription,
hooks: js.UndefOr[InserterHooks] = js.undefined
) extends Inserter with Hookable[DynamicInserter] {

Expand All @@ -69,7 +69,7 @@ class DynamicInserter(
)

ReactiveElement.bindSubscriptionUnsafe(element) { mountContext =>
insertFn(insertContext, mountContext.owner)
insertFn(insertContext, mountContext.owner, hooks)
}
}

Expand All @@ -78,7 +78,8 @@ class DynamicInserter(
}

override def withHooks(addHooks: InserterHooks): DynamicInserter = {
new DynamicInserter(initialContext, preferStrictMode, insertFn, addHooks.appendTo(hooks))
val newHooks = addHooks.appendTo(hooks)
new DynamicInserter(initialContext, preferStrictMode, insertFn, newHooks)
}

/** Call this to get a copy of Inserter with a context locked to a certain element.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@ import scala.scalajs.js
* We currently use it only for slotting elements into web components,
* but will likely use it more broadly later.
*
* NOTE: Currently hooks do not run on _some_ text nodes. They are also
* not run on some sentinel comment nodes, because only element nodes are
* slottable. So, this is fine for slotting purposes, but that's the
* kind of thing that will need a more principled contract if this API
* is to be used more widely.
*
* WARNING: Your hooks should not throw!
* Any thrown errors will be sent to Airstream unhandled errors.
*
Expand Down
11 changes: 6 additions & 5 deletions src/main/scala/com/raquo/laminar/nodes/Slot.scala
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.raquo.laminar.nodes

import com.raquo.airstream.core.AirstreamError
import com.raquo.laminar.inserters.{Hookable, Inserter, InserterHooks}
import org.scalajs.dom

Expand All @@ -18,11 +19,11 @@ class Slot(val name: String) {
case el: dom.Element =>
el.setAttribute("slot", name)
case text: dom.Text =>
dom.console.error(
s"Error: You are trying to insert a raw text node `${text.textContent}` into the `${name}` slot of <${parent.ref.tagName}>.\n" +
" - Cause: This is not possible. Only elements can be slotted.\n" +
" - Suggestion: Wrap your text node into span()"
)
AirstreamError.sendUnhandledError(new Exception(
s"Error: You tried to insert a raw text node `${text.textContent}` into the `${name}` slot of <${parent.ref.tagName.toLowerCase}>.\n" +
" - Cause: This is not possible: named slots only accept elements. Your node was inserted into the default slot instead.\n" +
" - Suggestion: Wrap your text node into `span()`"
))
case _ =>
() // Do nothing with comment nodes
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ object ChildReceiver {
}

def <--(childSource: Source[ChildNode.Base]): DynamicInserter = {
ChildInserter(childSource.toObservable, RenderableNode.nodeRenderable, hooks = js.undefined)
ChildInserter(childSource.toObservable, RenderableNode.nodeRenderable, initialHooks = js.undefined)
}

implicit class RichChildReceiver(val self: ChildReceiver.type) extends AnyVal {
Expand All @@ -38,7 +38,7 @@ object ChildReceiver {
)(
implicit renderable: RenderableNode[Component]
): DynamicInserter = {
ChildInserter(childSource.toObservable, renderable, hooks = js.undefined)
ChildInserter(childSource.toObservable, renderable, initialHooks = js.undefined)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ object ChildTextReceiver {
// #TODO[Perf] Test performance vs regular child.text, see if we need to improve this.
// This .asInstanceOf is safe because `textNodeRenderable` only applies if `TextLike` is `TextNode`.
val nodes = textSource.toObservable.asInstanceOf[Observable[TextNode]]
ChildInserter(nodes, RenderableNode.nodeRenderable, hooks = js.undefined)
ChildInserter(nodes, RenderableNode.nodeRenderable, initialHooks = js.undefined)
} else {
ChildTextInserter(textSource.toObservable, renderable)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,6 @@ object ChildrenCommandReceiver {
)(
implicit renderableNode: RenderableNode[Component]
): DynamicInserter = {
ChildrenCommandInserter(commands.toObservable, renderableNode, hooks = js.undefined)
ChildrenCommandInserter(commands.toObservable, renderableNode, initialHooks = js.undefined)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,15 @@ object ChildrenReceiver {
// Let me know if you have a compelling use case for this.

def <--(childrenSource: Source[immutable.Seq[ChildNode.Base]]): DynamicInserter = {
ChildrenInserter(childrenSource.toObservable, RenderableNode.nodeRenderable, hooks = js.undefined)
ChildrenInserter(childrenSource.toObservable, RenderableNode.nodeRenderable, initialHooks = js.undefined)
}

def <--[Component](
childrenSource: Source[immutable.Seq[Component]]
)(
implicit renderableNode: RenderableNode[Component]
): DynamicInserter = {
ChildrenInserter(childrenSource.toObservable, renderableNode, hooks = js.undefined)
ChildrenInserter(childrenSource.toObservable, renderableNode, initialHooks = js.undefined)
}

implicit class RichChildrenReceiver(val self: ChildrenReceiver.type) extends AnyVal {
Expand Down

0 comments on commit b59f019

Please sign in to comment.