e.target null in React 0.14.7

[ngbinh]

https://github.com/facebook/react/releases/tag/v0.14.7

seems to break react.js 0.10.4

    val onEmailChange: ReactEventI => Callback = { e =>
      scope.modState(_.copy(email = e.target.value))
    }

        ^.onChange    ==> backend.onEmailChange,

is broken because somehow e.target is null. Downgrade to react.js 0.14.6 seems to fix it.

[japgolly]

There was this thing React implemented a while back (0.13?) where events would have their properties wiped at the end of the callback unless you call .persist on them. If you were storing that callback and then having another thread (well "timeout") call it, then it would normal behaviour. But in your case you have onChange directly calling .modState so it should work.

This commit here looks related:
facebook/react@3285d83#diff-4bac2e331b5408363d4b836243e4a101

It's weird though because I'm using 0.14.7 in my work project and I haven't had this problem. Are you sure there's nothing else changed?

[ngbinh]

in term of react related, nothing changed! All I did to see the problem was upgrading to react.js to 0.14.7

[japgolly]

That's weird. Ok, need to figure out if problem with React.JS or scalajs-react. If problem with JS, you can confirm quick using a fiddle https://jsfiddle.net/reactjs/69z2wepo/ and if it's a bug, raise on facebook/react directly. If JS is ok then I suggest write a Scala.JS unit test to confirm and demonstrate.

[japgolly]

Oops that fiddle is 0.14.6. Oh well, just update the JS links to 0.14.7.

[ngbinh]

ok, will look into it. May take some time as I am on vacation.

[japgolly]

No worries. I use 0.14.7 too so I'll keep an eye out.

[ochrons]

After I updated SPA tutorial to use 0.14.7, I'm also seeing this same issue in TODO.scala

    def updateDescription(e: ReactEventI) =
      // update TodoItem content
      t.modState(s => s.copy(item = s.item.copy(content = e.target.value)))
.
.
          <.input.text(bss.formControl, ^.id := "description", ^.value := s.item.content,
            ^.placeholder := "write description", ^.onChange ==> updateDescription)),

e.target is null

Going back to 0.14.6 fixes the problem.

[japgolly]

Probably a React bug. We aren't doing anything special with events or modState.

[ochrons]

Seems like sort of expected behaviour in React (http://ludovf.net/reactbook/blog/reactjs-null-event-target.html). For some reason wasn't that visible before 0.14.7

[ochrons]

Modifying the code like this fixes the issue in 0.14.7, too

    def updateDescription(e: ReactEventI) = {
      val text = e.target.value
      // update TodoItem content
      t.modState(s => s.copy(item = s.item.copy(content = text)))
    }

[ngbinh]

hmm, that's strange. If it works, the previous version should work too.

[nafg]

Maybe they changed how aggressively / eagerly they recycle?

[japgolly]

Seems pretty deliberate: facebook/react#5840

This sucks because it's another one of those cases where you need to read the fine print of the docs to avoid a runtime error. Unfortunately, there's nothing I can do to prevent this at compile-time. We'll have to just either live with it or petition upstream to do something about it.

[nafg]

What about Callback.event helper that returns an event => Callback and
takes an event' => Callback where event' is a case class type in Scala
that's immediately populated from the raw JavaScript object? (Or that
simply calls .persist first, named accordingly.)

On Thu, Feb 25, 2016, 3:29 AM David Barri [email protected] wrote:

Closed #255 #255.

—
Reply to this email directly or view it on GitHub
#255 (comment).

[japgolly]

React actively discourages using .persist so I wouldn't want to build a helper on it. Instead how about tacking a method onto events that allows them to immediately extract the portion they need like

def extract[A, B](f: this.type => A)(g: A => B): B =
  g compose f

which when used looks like:

    def updateDescription(e: ReactEventI) =
      e.extract(_.target.value)(text =>
        t.modState(s => s.copy(item = s.item.copy(content = text)))

It's just a bit of sugar to avoid writing:

    def updateDescription(e: ReactEventI) = {
      val text = e.target.value
      t.modState(s => s.copy(item = s.item.copy(content = text)))
    }

I'm not sure it's worth it.

[nafg]

Yeah, I'd rather it would just automatically lift it to a Scala case class
that corresponds to the whole event. So it does save boilerplate, but even
more importantly, it helps not expose you to that sort of bug.

On Thu, Feb 25, 2016, 7:40 PM David Barri [email protected] wrote:

React actively discourages using .persist so I wouldn't want to build a
helper on it. Instead how about tacking a method onto events that allows
them to immediately extract the portion they need like

def extract[A, B](f: this.type => A)(g: A => B): B =
g compose f

which when used looks like:

def updateDescription(e: ReactEventI) =
  e.extract(_.target.value)(text =>
    t.modState(s => s.copy(item = s.item.copy(content = text)))

It's just a bit of sugar to avoid writing:

def updateDescription(e: ReactEventI) = {
  val text = e.target.value
  t.modState(s => s.copy(item = s.item.copy(content = text)))
}

I'm not sure it's worth it.

—
Reply to this email directly or view it on GitHub
#255 (comment)
.

[nafg]

Also, ==> could be changed to not take a raw ReactEventI etc. but some new
EventCallback type (with deprecated backward compatibility) so users don't
have to do anything extra to be shielded from null.
(Obviously the philosophy about null is different in JavaScript land than
in Scala land.)
In a sense it's very similar to why scala-js-react wraps state in a
Callback: it's a value that's dynamically scoped, and in Scala we prefer
safety.

On Thu, Feb 25, 2016, 10:18 PM Naftoli Gugenheim [email protected]
wrote:

Yeah, I'd rather it would just automatically lift it to a Scala case class
that corresponds to the whole event. So it does save boilerplate, but even
more importantly, it helps not expose you to that sort of bug.

On Thu, Feb 25, 2016, 7:40 PM David Barri [email protected]
wrote:

React actively discourages using .persist so I wouldn't want to build a
helper on it. Instead how about tacking a method onto events that allows
them to immediately extract the portion they need like

def extract[A, B](f: this.type => A)(g: A => B): B =
g compose f

which when used looks like:

def updateDescription(e: ReactEventI) =
  e.extract(_.target.value)(text =>
    t.modState(s => s.copy(item = s.item.copy(content = text)))

It's just a bit of sugar to avoid writing:

def updateDescription(e: ReactEventI) = {
  val text = e.target.value
  t.modState(s => s.copy(item = s.item.copy(content = text)))
}

I'm not sure it's worth it.

—
Reply to this email directly or view it on GitHub
#255 (comment)
.

[japgolly]

The problem is that events are mutable and recycled by React, presumably because it's important for performance. Making a wrapper case class or changing ==> would only solve it if we took an immutable copy of the event which would just add back (and more) the performance impact that motivated FB to make this change in the first place. I'm not keen on imposing a performance degradation on downstream projects. I think calling persist / extracting values yourself is the least worst of the options.