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

Unreliable off() event handling #1371

Closed
3 tasks done
kentico-ericd opened this issue Aug 27, 2024 · 6 comments
Closed
3 tasks done

Unreliable off() event handling #1371

kentico-ericd opened this issue Aug 27, 2024 · 6 comments

Comments

@kentico-ericd
Copy link

kentico-ericd commented Aug 27, 2024

Prerequisites

  • I am running the latest version
  • I checked the documentation and found no answer
  • I checked to make sure that this issue has not already been filed

💥 Demo Page

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

Explanation

  • What is the expected behavior?
    You can call off('add') between adding of multiple tags to prevent the event from triggering. In the demo, the expected console output would be
"adding tags without off()"
"1"
"2"
"adding tags with off()"
  • What is happening instead?
    It looks like calling off('add') at any point in the example prevents the event from triggering for all tags. If you run the demo as-is, the event is not triggered at all. If you comment the off('add') line and re-run, all the tags show in the console.

  • What error message are you getting?

@kentico-ericd
Copy link
Author

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
Copy link
Owner

yairEO commented Aug 28, 2024

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

@yairEO yairEO closed this as completed Aug 28, 2024
@yairEO yairEO reopened this Aug 28, 2024
@kentico-ericd
Copy link
Author

kentico-ericd commented Aug 28, 2024

@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 yairEO closed this as completed in f7c7a84 Aug 28, 2024
@yairEO
Copy link
Owner

yairEO commented Aug 28, 2024

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

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 😅 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
Copy link
Owner

yairEO commented Aug 28, 2024

Good to hear it was resolved :)

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

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

2 participants