cyclejs icon indicating copy to clipboard operation
cyclejs copied to clipboard

dom ownerTarget approach not backwards compatible with v20

Open ntilwalli opened this issue 6 years ago • 1 comments

Code to reproduce the issue:

import {run} from '@cycle/rxjs-run'
import {combineLatest, of, merge} from 'rxjs'
import {map} from 'rxjs/operators'

import {makeDOMDriver} from '@cycle/dom/lib/cjs/rxjs'
import {withState as onionify} from '@cycle/state'
import {div, span, ul, li, br} from '@cycle/dom'

function view(state) {
  const arr = ['one', 'two', 'three']
  return div([
    div([
      'item: ',
      state.item || 'none'
    ]),
    br([]),
    ul(arr.map(x => {
      return li('.appResult', {props: {element_name: x}}, [
        span([x])
      ])
    }))
  ])
}

function Items(sources) {
  const state$ = sources.Onion.stream
  return {
    DOM: state$.pipe(
      map(view)
    ),
    Onion: merge(
      sources.DOM.select('.appResult').events('click').pipe(
        map(ev => ev.ownerTarget.element_name),
        map((item: any) => state => {
          return {
            ...state,
            item
          }
        })
      ), 
      of(state => {
        return {
          ...state
        }
      })
    )
  }
}

function main(sources) {
  const state$ = sources.Onion.stream
  const items: any = Items(sources)
  return {
    DOM: combineLatest(state$, items.DOM).pipe(
      map(([state, items]) => {
        return div([
          items
        ])
      })
    ),
    Onion: merge(
      of((state: any) => {
        const x = 1
        return {
          ...state
        }
      }),
      items.Onion
    )
  }
}

const wrappedMain = onionify(<any>main, 'Onion')


run(wrappedMain, {
  DOM: makeDOMDriver(`#app-main`)
})

In dom v20 the ownerTarget property on the event stayed consistent between emitting event and calling the reducer. In v22, the ownerTarget property is correct when the events stream emission takes place but is overwritten by the time the reducer is run. This is because the reducer is run on the next turn of the event-loop, so by time the reducer is called the new simulated bubbling code in EventDelegator has overwritten that property multiple times as the bubbling was simulated. I don't consider this a bug as much as a backwards-incompatibility. In v20 it was valid to pass the event directly to the reducer and inquire the value within the reducer. In v22 the ownerTarget value must be extracted immediately upon event-stream emission (synchronously) so it can directly be included in the closure associated with the reducer. If this is not a bug then this change should be documented.

ntilwalli avatar Aug 02 '19 13:08 ntilwalli

This could possibly be made more backwards compatible by simply shifting where ownerTarget is set like so:

  private doBubbleStep(
    eventType: string,
    elm: Element,
    rootElement: Element | undefined,
    event: CycleDOMEvent,
    listeners: PriorityQueue<Destination> | Array<Destination>,
    useCapture: boolean,
    passive: boolean
  ): void {
    if (!rootElement) {
      return;
    }
    var self = this;
    listeners.forEach(dest => {
      if (dest.passive === passive && dest.useCapture === useCapture) {
        const sel = getSelectors(dest.scopeChecker.namespace);
        if (
          !event.propagationHasBeenStopped &&
          dest.scopeChecker.isDirectlyInScope(elm) &&
          ((sel !== '' && elm.matches(sel)) ||
            (sel === '' && elm === rootElement))
        ) {
          self.mutateEventCurrentTarget(event, elm);
          preventDefaultConditional(
            event,
            dest.preventDefault as PreventDefaultOpt
          );
          dest.subject.shamefullySendNext(event);
        }
      }
    });
  }

ntilwalli avatar Aug 03 '19 21:08 ntilwalli