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

Add event should not get triggered for existing tags upon initialization #1362

Closed blutorange closed 3 months ago

blutorange commented 4 months ago

Prerequisites

Demo Page

https://jsbin.com/zijiwivupe/edit?html,js,output

It should not get triggered and no output should be printed.

image

Explanation

I am not sure if this is the expected behavior (?), but it has tripped us up. Event listeners for the add event get called when tagify is initialized and the input already contains a value. We only want to listen for newly added tags, not existing tags that were present already upon page load. This happens even though the event listener is registered after tagify was initialized.

var input = document.querySelector('input')
var tagify = new Tagify(input, {});
tagify.on('add', () => {
  document.querySelector("#out").textContent += "\nevent<add>";
});

The add event listener should only be invoked for newly added tags.

The add event listener is invoked for already existing tags when tagify is initialized; once for each existing tag.

Again I'm not sure if this is intentional, but the cause for this behavior is that (a) tagify uses its addTags method when a new instance is created; and (b) that this method ends up calling postProcessNewTagNode. That method triggers the add event asynchronously; and as a result, event listeners registered after tagify was initialized are invoked as well.

https://github.com/yairEO/tagify/blob/dba7a6b7e84baf1d57147bcaae2b64f84a3c211a/src/tagify.js#L1297C1-L1299

We wrap tagify as a PrimeFaces component. The add event is translated into a Faces event. When the user adds a new tag, this results in an AJAX request made to the backing Java bean, which handles the newly added tag and updates the data in the database. This should only occur for newly added tags, not tags that were already present in the database.

The only workaround I could come up with is to add our event listeners asynchronously as well, so that our code gets pushed to the event loop queue after the tagify code that triggers the event.

var input = document.querySelector('input')
var tagify = new Tagify(input, {});
setTimeout(() => tagify.on('add', () => {
  document.querySelector("#out").textContent += "\nevent<add>";
}));
chuttam commented 4 months ago

Yes! We just encountered the same trouble and rolled back to 4.26.5

Could this commit be where the unexpected behaviour (initial callout to add event on tagger initialization when tags already exist) was introduced?

blutorange commented 3 months ago

@chuttam Yes, judging from my debugging session, that commit should be the immediate cause of this issue. According to the code comments, it was done for a different reason, so I guess triggering event listeners on initialization was probably not intended.

The easiest solution to me seems to be to pass a flag to the internal method from the constructor so that is does not invoke event listeners when tagify is initialized. But let's wait for a response from the author.

yairEO commented 3 months ago

will be fixed ASAP (today). Anyway, you can use the better option (IMO) - change event

I am sorry, I was on a very long vacation

blutorange commented 3 months ago

No problem, I hope you had a good time. Thanks for fixing it!

yairEO commented 3 months ago

I had a great time in Italy :) does the new version from yesterday works well?

blutorange commented 3 months ago

Sounds like a good time indeed, fresh pizza and olives : ) I just tested the new release with our component, works smoothly and doesn't trigger the event prematurely. Thanks!

yairEO commented 3 months ago

Happy to hear you are satisfied with the component

Yes, there was pizza but I wasn't so impressed with the quality and nobody in Italy speaks English so it was a bit difficult, but lots of hiking and lakes & rivers swimming :)

chuttam commented 3 months ago

Thanks for the speedy response @yairEO despite being away. Vacations and breaks really help clear the mind, especially when you're the only one curating a very useful tool so many are dependant on.