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

TargetOps assume incorrect type for target and currentTarget #93

Open
cornerman opened this issue Nov 14, 2017 · 14 comments
Open

TargetOps assume incorrect type for target and currentTarget #93

cornerman opened this issue Nov 14, 2017 · 14 comments

Comments

@cornerman
Copy link
Member

cornerman commented Nov 14, 2017

Currently, we have InputEvent for representing all form events. But InputEvent assumes the wrong type of the eventTarget and casts to HTMLInputElement. Even events like onInput can trigger on elements like <input>, <textarea>, <select>, or <... contenteditable>. The other events have different possible element targets.

We try to tackle this in scala-dom-types and for most events we can only assume html.Element for typing the eventTarget. It is better than type EventTarget, but the problem is, you still do not have access to the .value, .valueAsNumber, .checked properties (we currently have that).

Four options for not having to write .asInstanceOf[HtmlInputElement].value:

  1. Define helper methods like targetValue, targetValueAsNumber, targetChecked on the event. Something like this:
  implicit class InputEventWithValue(val ev: InputEvent) extends AnyVal {
    import dom.html

    def targetValue: String = ev.target match {
      case input: html.Input => input.value
      case textarea: html.TextArea => textarea.value
      case select: html.Select => select.value
      case elem => elem.textContent // for contenteditable
    }

    def targetValuesAsNumber: Int = ...
    def targetChecked: Boolean = ...
  }

I kind of like this option, as it is short and will work for any element that could be the target. But has to make certain assumptions, especially for implementing something like targetValueAsNumber, targetChecked.

  1. Define helper methods targetT as a shortcut for target.asInstanceOf:
  implicit class EventWithTypedTarget(val ev: dom.Event) extends AnyVal {
    def targetT[T <: dom.EventTarget]: T = ev.target.asInstanceOf[T]
  }

More flexible, as is asInstanceOf, but not typesafe. If you ever come to change the embedding element, it will fail.

  1. Define helper methods like targetInput, targetTextarea, targetSelect. Similar like the one before, but saving you from importing the type parameter.
  implicit class EventWithTypedTarget(val ev: dom.Event) extends AnyVal {
    def targetInput: HtmlInputElement = ev.target.asInstanceOf[HtmlInputElement] 
    def targetTextarea: HtmlTextAreaElement = ???
  }

Nicer to write than 2), but same drawback.

  1. Similar to what Override currentTarget scala-js/scala-js-dom#228 does. We can type the currentTarget of the event, as it is always the type of the element, to which the event listener was attached. So on the user side, we hopefully could write something like input(onChange(_.currentEvent.value) --> strHandler).

I like this option most, as it is typesafe and does not make us handle specific use-cases but the general use-case.

What do you think?

@fdietze
Copy link
Member

fdietze commented Nov 14, 2017

I'm in favor of version 1. It's simple and flexible.

Version 2 requires more knowledge and imports about the required HTML elements.
Both version 2 and 3 need additional casting if you want to extract a number from the value.

@fdietze
Copy link
Member

fdietze commented Nov 14, 2017

Related: scala-js/scala-js-dom#228

@cornerman
Copy link
Member Author

Both version 2 and 3 need additional casting if you want to extract a number from the value.

This is not true, valueAsNumber is defined on HtmlInputElement, so you can just use it after casting to the right element type.

@fdietze
Copy link
Member

fdietze commented Nov 16, 2017

Oh, you are right!

@cornerman
Copy link
Member Author

cornerman commented Nov 16, 2017

Related: scala-js/scala-js-dom#228

For something like this, we would need scala-dom-types to capture the embedding element-tag. But I am not sure, whether a sane syntax is possible without implicit function types in dotty. Or we create a macro for the event properties, that injects this relationship into the code. @raquo: do you have an idea?

This would then be for currentTarget and not for target, as currentTarget will always be the element on which the event listener was attached.

@raquo
Copy link

raquo commented Nov 17, 2017

Hrm. In Scala Dom Builder & Laminar I have properly typed references to real JS nodes:

val scalaNode = div() // SomeType[dom.html.Div]
val jsNode = scalaNode.ref // dom.html.Div

div.apply accepts a list of Modifier[dom.html.Div]. So in this setup all I need is to make onClick --> handler grab that dom.html.Div type using something like def -->[El <: dom.html.Element]: Modifier[El].

Having that type param available inside the --> method should solve the problem by casting the currentTarget like we did with ElementTargetEvent in Scala DOM Types, or via some other mechanism, I think...

I haven't actually tried this yet, there could be some problems e.g. in my case Modifier[-T] is contra-variant, so type inference might not work as desired.

@cornerman
Copy link
Member Author

Good idea! That might actually work. Will give it a try!

@cornerman
Copy link
Member Author

Hmm, in general this works and the types of the element get inferred correctly. I have a VNodeBuilder[T <: dom.Element], which only accepts VDomModifierBuilder[T, _ <: VDomModifier].

So then I have something like this for subscribing events to an Observer (a sink is just a wrapper around an observer in outwatch):

  def -->[Elem <: dom.Element](sink: Sink[_ >: Event]): VDomModifierBuilder[Elem, Emitter] = ...

When I then try to make use of the Elem type in an argument, type inference does not work anymore and I have to provide the type of the element. So this looks something like this:

  def --->[Elem <: dom.Element, O](f: Event with TypedCurrentTargetEvent[Elem] => O)(sink: Sink[_ >: O]): VDomModifierBuilder[Elem, Emitter] = ...

@cornerman
Copy link
Member Author

cornerman commented Nov 19, 2017

On top of the scala-dom-types branch I have started to build a dom.Element type-aware builder here.

Currently the best I can come up with for the currentTarget is this:

input(
    input.event(onChange).map(_.currentTarget.value) --> strHandler
)

As opposed to the following, which does not compile as onChange alone does not know that it is inserted in an input element and therefore cannot access .value on currentTarget:

input(
    onChange.map(_.currentTarget.value) --> strHandler
)

So, all old code using onChange or onClick would still work, but you can opt-in to type the currentTarget of your event by doing textarea.event(onChange) or div.event(onClick). This is better than .asInstanceOf, but it still feels verbose to me.

Update: Ok, this is also possible:

input(
    _(onChange).map(_.currentTarget.value) --> strHandler
)

@fdietze
Copy link
Member

fdietze commented Nov 20, 2017

Seems like we need a macro for the tags like input() which injects its type into the emitters. 🙄

@cornerman
Copy link
Member Author

I hope not, because a macro would not really work with scala-dom-types as it is now. Then the definitions of the tags like input or textarea would already need to be macro definitions. I am not sure whether we have a general case here...also thinking compile time.

And this problem is only because of how we build our eventemitters. A simple version like this could easily work:

val node = div("bla")
val events = node.event(onChange) : Observable[Event with TypedCurrentTargetEvent[dom.html.Div]]

@raquo
Copy link

raquo commented Nov 21, 2017

@cornerman What is _ in _(onChange) in your example? What's the type of _(onChange).map(_.currentTarget.value) --> strHandler?

Edit: Nevermind, I missed the link to your branch. I see it now: type Modifier[E <: dom.Element] = Context[E] => VDomModifier

@cornerman
Copy link
Member Author

@raquo oh, that was an example of how "short" i could get the workaround to make an extra step to type the current target.

I have made the modifiers a function type Modifier[E <: dom.Element] = Context[E] => VDomModifier. And then _ is the Context, where I call the apply method, to get a better EmitterBuilder with a element-aware event type.

I am not really happy with it, but I cannot get type inference to help me in the case of events. Because I need the element type at the EmitterBuilder, but I can only infer it for the whole expression, which is after the --> is applied.

cornerman added a commit to cornerman/outwatch that referenced this issue Dec 2, 2017
cornerman added a commit to cornerman/outwatch that referenced this issue Dec 3, 2017
@cornerman cornerman changed the title InputEvent assumes incorrect type for target TargetOps assume incorrect type for target Jan 12, 2018
@cornerman cornerman changed the title TargetOps assume incorrect type for target TargetOps assume incorrect type for target and currentTarget Jan 12, 2018
@cornerman
Copy link
Member Author

Behaviour has changed with #139, but still just assumes InputElement

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

3 participants