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

dom ownerTarget approach not backwards compatible with v20 #895

Open
ntilwalli opened this issue Aug 2, 2019 · 1 comment
Open

dom ownerTarget approach not backwards compatible with v20 #895

ntilwalli opened this issue Aug 2, 2019 · 1 comment

Comments

@ntilwalli
Copy link
Contributor

ntilwalli commented Aug 2, 2019

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
Copy link
Contributor Author

ntilwalli commented Aug 3, 2019

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);
        }
      }
    });
  }

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

1 participant