yairEO / tagify

🔖 lightweight, efficient Tags input component in Vanilla JS / React / Angular / Vue
https://yaireo.github.io/tagify/
Other
3.55k stars 436 forks source link

Unreliable off() event handling #1371

Closed kentico-ericd closed 2 months ago

kentico-ericd commented 2 months ago

Prerequisites

💥 Demo Page

https://jsbin.com/badovoduzu/1/edit?html,js,console,output

Explanation

"adding tags without off()"
"1"
"2"
"adding tags with off()"
kentico-ericd commented 2 months ago

Note: In my application I fixed this by setting state.blockChangeEvent but it's not ideal:

/**
 * Enables or disables to add/remove event handlers for the tags Tagify element.
 * @param enable If true, the events are enabled. Otherwise, they are disabled.
 */
const toggleTagEvents = (enable: boolean) => {
  if (enable) {
    // @ts-ignore
    tagsElement.state.blockChangeEvent = false;
    tagsElement.on('add', addTag).on('remove', removeTag);
  }
  else {
    // @ts-ignore
    tagsElement.state.blockChangeEvent = true;
    tagsElement.off('add', addTag).off('remove', removeTag);
  }
};
yairEO commented 2 months ago

because there is a slight delay before the event is actually emitted, so basically this is the actual sequence of code:

tagify.on('add', onAddTag)
tagify.off('add', onAddTag)
tagify.addTags('1', true)
tagify.addTags('2', true)
tagify.addTags('3', true)
tagify.addTags('4', true)

By the time the addTags method finished, all it "knows" is that the add event listener was canceled:

https://github.com/yairEO/tagify/blob/master/src/tagify.js#L1301-L1308

This is because the tags are all added together as a fragment:

https://github.com/yairEO/tagify/blob/master/src/tagify.js#L1417-L1423

why would you want to add and then remove the event listener immediately?


I can think how to solve this if there is good justification for such a use case of listening and then un-listening to an event immediately

kentico-ericd commented 2 months ago

@yairEO In my application there is a tag input where the user can apply tags to support tickets. The handler needs to be registered when the user manually adds a tag, to send out a request to Dynamics 365. But when a user loads a support ticket, all pre-existing tags are retrieved from Dynamics and added to the Tagify input, and these should not trigger a request to Dynamics. So, I've created this toggleTagEvents function for loading the existing tags:

toggleTagEvents(false)
// await Dynamics request, add tags to input
toggleTagEvents(true)

It's a bit odd that the on and off functions are not marked async but effectively function that way... is there a way to refactor the setTimeout implementation and make them async instead? Or is there another way to add the tags to the input without triggering the handlers?

yairEO commented 2 months ago

I've fixed it :)

https://github.com/yairEO/tagify/releases/tag/v4.31.2

I think the change event is better than the add event because it provides a single event when tags where added or removed, and you don't really care which tags were what or how many, because many times you only need to reflect the current state of the component to the backend, to be synced

kentico-ericd commented 2 months ago

I can confirm that updating has resolved my issue, thanks! I'm going to leave the state.blockChangeEvent in there anyways because I'm nervous about the issue somehow happening again :sweat_smile: It caused a pretty big issue where support tickets had duplicated tags in the hundreds, but went unnoticed as the Tagify element only showed one instance of each tag

yairEO commented 2 months ago

Good to hear it was resolved :)

anyway, you can place safeguards in the backend instead of the frontend to bulletproof this