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

Null cache exception in FlatMap #188

Closed
basimkhajwal opened this issue Jul 19, 2019 · 8 comments
Closed

Null cache exception in FlatMap #188

basimkhajwal opened this issue Jul 19, 2019 · 8 comments

Comments

@basimkhajwal
Copy link

I'm currently updating a project that was written under Binding.scala 10.0.3 (and compiled/ran fine) to 11.8.1. Updating the library caused a null error to surface that previously wasn't there.

Specifically, in line 408 of Binding.scala, oldCache is null.

Looking through the changes to the relevant code since 10.0.03, I can see that this commit added a cache which is initialised to null.

Either there was some invariant that meant that oldCache should never be null (in which I need to fix my code) or a null check needs to be made.

@Atry
Copy link
Collaborator

Atry commented Jul 20, 2019

Could you provide a reproduction code, e.g. a link to https://scalafiddle.io/ ?

@basimkhajwal
Copy link
Author

Working code under 10.0.3: https://scalafiddle.io/sf/bzRRp4g/0
Error under 11.8.1: https://scalafiddle.io/sf/HcOpljv/0

On a side note, I can see the approach that I took isn't very nice so I wanted to ask if there was a better to do what I intend (keep a cache that can both be mutated externally and whenever a cache miss occurs)

@Atry
Copy link
Collaborator

Atry commented Jul 20, 2019

According to the Scaladoc for the value method: https://javadoc.io/page/com.thoughtworks.binding/binding_2.11/latest/com/thoughtworks/binding/Binding$$Vars.html#value:scala.collection.mutable.Buffer[A]

This method must not be invoked inside a @dom method body or a Binding { ... } block..

@Atry
Copy link
Collaborator

Atry commented Jul 20, 2019

It does not make sense to change a Var or Vars in a data binding expression that depends on itself. It's like a barber paradox.

As a workaround, you can delay the change by wrapping the value calls into a Future:

import com.thoughtworks.binding.Binding._
import com.thoughtworks.binding._

def expensiveOperation(i: Int): Object = "Dummy"

// Cache the operations performed, a Var was needed
// because it could be mutated elsewhere
val objectCache: Vars[(Int,Object)] = Vars.empty

// Index mutated through UI
val selectedIdx: Var[Int] = Var(0)

// Value which is then used to render DOM
val objectValue: Binding[Object] = Binding {
  val idx = selectedIdx.bind
  val cache = objectCache.bind
  
  cache.find { case (i,_) => i == idx} match {
    case Some((_,obj)) => obj
    case None => {
      val newObject = expensiveOperation(idx)
      import concurrent.ExecutionContext.Implicits._
      concurrent.Future {
        objectCache.value += ((idx, newObject))
      }
      newObject
    }
  }
}

Binding {
  println(objectValue.bind)
}.watch()

https://scalafiddle.io/sf/HcOpljv/1

Even if you delay the change, recursive data binding expression is still likely buggy, because it may cause endless recalculation.

@Atry
Copy link
Collaborator

Atry commented Jul 20, 2019

I know AngularJS automatically delay changes, allowing recursive data binding. This approach causes AngularJS's infamous stability problem.

In Binding.scala, recursive data binding requires explicit delay execution with a Future or setTiemout. You take the risk when you mean it.

@basimkhajwal
Copy link
Author

Right, I understand.

One thing I'm still unsure about is the typical pattern for input elements which goes like:

import org.scalajs.dom.html

@dom
def inputElement(data: Var[String]): Binding[html.Input] = {
  <input oninput={e:Event => data.value = e.target.asInstanceOf[html.Input].value}
    value={data.bind} />
}

This is still a recursive binding that works however it still invalidates the principle that value_= should never be called within a @dom expression.

Moving the oninput to an external function kind of resolves it but it still effectively does the same thing (recursive) so I'm still unsure as to how to properly handle this situation (if there is an alternative).

@Atry
Copy link
Collaborator

Atry commented Jul 20, 2019 via email

@Atry
Copy link
Collaborator

Atry commented Jul 20, 2019 via email

Atry added a commit that referenced this issue Jul 21, 2019
Detect reentry problem to provide better error message for #188
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