naver / egjs

Javascript components group that brings easiest and fastest way to build a web application in your way.
https://naver.github.io/egjs
Other
937 stars 81 forks source link

Reusing trigger customEvent object #458

Closed vreality64 closed 7 years ago

vreality64 commented 7 years ago

I'll mention 3 things.

  1. Situation I been through.
  2. The thing what I expected.
  3. Solution.

Situation I been through

I have one controller and two modules. everything is class inherited from eg.Component.

// Controller, Module A, Module B
var instance = new eg.Class.extend(eg.Component, {
  // ...
});

I tried handle custom event state between modules and controller using customEvent.stop() function. But stop was not worked.

Trigger Docs: If the stop() method is called by a custom event handler, it will return false and prevent the event from occurring.

Here are code.

// Controller has Module A. Module A has Module B.
// every callback function is wrapped `$.proxy` with `this` context.
// this.module.on('eventName', $.proxy(callback, this));

// In Controller
moduleA.on('beforeSelect', function (event) {
  event.stop();
});

// In Module A
moduleB.on('beforeSelect', function (event) {
  this.trigger('beforeSelect', event);  // reuse event object
});

// In Module B
this.on('click', function (event) {
  if (this.trigger('beforeSelect')) {
    this.select('data');
  }
})

The thing what I expected

Code describes that if controller calls event.stop() then doesn't execute moduleB.select(). because stop() makes trigger return false.

I think reusing custom event is common case. normally custom event has data, so it was broadcasted to system.

But when it reused, isCanceled variable was overridden.

trigger: function(eventName, customEvent) {
  // event(data) Object are alive.
  customEvent = customEvent || {};

  // ...
  // isCanceled variable is overridden.
  var isCanceled = false;

  // ...
  return !isCanceled;
},

If intended, it is notable.

Solution

I changed middle module. not re-used event object

// [Before] In Module A
moduleB.on('beforeSelect', function (event) {
  this.trigger('beforeSelect', event);  // reuse event object
}

// [Changed] In Module A
moduleB.on('beforeSelect', function (event) {
  var stop = !this.trigger("beforeSelect"); // not re-used event object
  if (stop) {
    event.stop();
  }
}

I made JS fiddle example. you can reproduce my situation.

Question

I can't sure what is right, so just want to know decision history. what philosophy / thoughts leads this conclusion?

mixed commented 7 years ago

@VReality64 Thank you for your report. I am going to anwser it soon. 😄

mixed commented 7 years ago

I think that This case is very important issue. Sometimes Component has not to fire event. So if trigger add option like boolean for event fire condition. I think differently It is duplicate feature(stop method). So. It need more idea.

sculove commented 7 years ago

Issue moved to naver/egjs-component #19 via ZenHub