I've a problem with my binding event, I would like to remove each event when change event is fired. It's working fine without a loop.

this.objectInstance.on('selected', () => {
    document.querySelectorAll('select').forEach((el) => {
        this.test = this.fontFamilyHandler.bind(this, el.getAttribute('id'));
        el.addEventListener('change', this.test);
    }) 
});

this.objectInstance.on('deselected', () => {
    document.querySelectorAll('select').forEach((el) => {
        el.removeEventListener('change', this.test);
    })
});

fontFamilyHandler = (key, evt) => {
    this.objectInstance.set(key, evt.target.value);
    this.canvas.requestRenderAll();
}

Do you have any idea why it's doesn't work ?

CodePudding user response：

In every iteration, you are overwriting this.test with the next listener. So at the end, this.test will contain the last listener attached and therefore it can only remove that one.

I can see two solutions here:

1. You could save all of the listener functions and then remove them again, mapping the element to the listener:

this.objectInstance.on('selected', () => {
    this.test = new WeakMap();
    document.querySelectorAll('select').forEach((el) => {
        const handler = this.fontFamilyHandler.bind(this, el.getAttribute('id'));
        this.test.set(el, handler);
        el.addEventListener('change', handler);
    }) 
});

this.objectInstance.on('deselected', () => {
    document.querySelectorAll('select').forEach((el) => {
        if (!this.test.has(el)) continue;
        el.removeEventListener('change', this.test.get(el));
    })
});

(Note: using just an array would not be stable because if the number of <select> elements in the document changes, the indices would get out of sync.)

(Note: It would also work with Map instead of WeakMap, but the latter allows earlier garbage-collection of any <select> element that gets removed from the DOM.)

2. Use the same function for all elements, instead of binding every time. Since you access the same element you are putting the listener on, and it's a <select> which means the change event will only ever come from the <select> itself as it cannot have any children that would be able to send change events, using event.target.id works too:

const eventHandler = event => this.fontFamilyHandler(event.target.id, event);

this.objectInstance.on('selected', () => {
    document.querySelectorAll('select').forEach((el) => {
        el.addEventListener('change', eventHandler);
    }) 
});

this.objectInstance.on('deselected', () => {
    document.querySelectorAll('select').forEach((el) => {
        el.removeEventListener('change', eventHandler);
    })
});

(Note: el.id is easier than el.getAttribute('id') and does the same - see Element#id.)

3. Or, you could use the fontFamilyHandler directly and change it to read the ID from the element itself.

this.objectInstance.on('selected', () => {
    document.querySelectorAll('select').forEach((el) => {
        el.addEventListener('change', this.fontFamilyHandler);
    }) 
});

this.objectInstance.on('deselected', () => {
    document.querySelectorAll('select').forEach((el) => {
        el.removeEventListener('change', this.fontFamilyHandler);
    })
});

// I removed the `key` argument here
this.fontFamilyHandler = evt => {
    this.objectInstance.set(evt.target.id, evt.target.value);
    this.canvas.requestRenderAll();
}

CodePudding user response：

From the above comment ...

"One needs to see surrounding code in order to get an understanding of the actual this the OP is dealing with ... I'm especially curious about fontFamilyHandler = (key, evt) => { /* ... */ } where this function floats freely versus line 3 where all of a sudden one sees this.fontFamilyHandler.bind(this, el.getAttribute('id'));"

Until then the next provided answer assumes kind of a class syntax/system for a hypothetical MyType class.

Of cause the binding as with ...

this.fontFamilyHandler.bind(this, el.getAttribute('id'));

... is not necessary at all because the OP already does access the event.target within this handler which equals the above el ...

fontFamilyHandler = (key, evt) => {
  this.objectInstance.set(key, evt.target.value);
  this.canvas.requestRenderAll();
}

Thus, binding each element's id is not necessary since this property can be accessed via evt.target.id as much as the value already via evt.target.value.

In the end the code will boil down to something much more readable like ...

//class MyType {
//  constructor() {
//    this.objectInstance = { on: () => {} };

    this.objectInstance.on('selected', () => document
      .querySelectorAll('select')
      .forEach(el =>
        el.addEventListener('change', this.fontFamilyHandler)
      )
    );
    this.objectInstance.on('deselected', () => document
      .querySelectorAll('select')
      .forEach(el =>
        el.removeEventListener('change', this.fontFamilyHandler)
      )
    );

    this.fontFamilyHandler = ({ target }) => {
      this.objectInstance.set(target.id, target.value);
      this.canvas.requestRenderAll();
    };

//  }
//}