Controlled component behavior in React wrapper

[agusterodin]

Instead of relying on callbacks to gather information about what happened as far as tag additions/changes/deletion, the tag input should be driven off of React state (ie: an array of tags as a single source of truth). Right now the source of truth is self contained within the tagify instance itself. Right now it behaves as an uncontrolled component whereas a controlled component would be ideal!

To make this possible, you would need to add a function that lets you manually set the contents of the tag input array (pass an array and it will change the contents of the tag input to reflect this new array).

It would also involve adding an option to instance factory that tells the library not to automatically add/remove/change the tag in its own internal tag state, but still provide information on what were to happen in handler functions so that we are given the necessary information to know how to alter the react state (which in turn will cause the controlled tagify component to be updated based on the array from state we feed into it).

[agusterodin]

This "controlled" behavior could also greatly enhance the experience with other view library/framework wrappers

[agusterodin]

The only super confusing part would be how to handle animations during state changes, especially since tagify would no longer have any idea of identity (especially if you have duplicates enabled and have duplicate tags). The animations make everything a lot harder.

I have seen other libraries that let you manually set the complete contents like the "setTokens" method this: http://sliptree.github.io/bootstrap-tokenfield/#methods which I saw here https://github.com/yairEO/tagify/issues/45. It doesn't look like they have animations though.

[agusterodin]

Also, rendering things onto the DOM between state changes would require either: a manual deletion and recreation of all tag DOM elements on every state change, intelligently determining how/if to mutate the DOM by yourself, or intelligently determining how/if to mutate the DOM with something like React.

At that point having this library operate in a "controlled" manner would likely cause you to need the assistance of a view library which kinda makes me think that the only elegant way to achieve this would be to build the library from the ground up in React.

These are just my thoughts. I have written react wrappers myself but this scenario is super awkward. I would be willing to help out if needed.

[agusterodin]

You could have it so that state is formed as such:

<Tagify tags={[{guid: 234asdfas, value: "testvalue"}, {guid: abc23452, value: "anothertestvalue"}]} />

That way identity can be maintained for animation purposes, especially when duplicates are enabled. This guid information would be provided along with the onAdd, onChange, onDelete events.

Also obviously the onAdd, onChange, and onDelete handlers would fire only when something was modified directly inside the tag input/list and not for additions/modifications/deletions that were made as a result of react passed in state changes.

[agusterodin]

Like where it can operate like this for example

const [tags, setTags] = useState(["Kevin", "Box"])


return(
<div>
<div onClick={() => setTags([])>Clear All</div>
<Tags
      value={tags}
      onChange={(event) => {
          setTags(event.target.value) 
      }}
    />
</div>
)

[yairEO]

Currently when you update the value prop of the <Tags> React component, it internally calls loadOriginalValues() method, which clean everything up and re-create all tags.

it's not intelligent at all, but in terms of performance totally negligible because I cannot imagine thousands of tags exists and re-created.

I think that below a couple hundred tags it's totally fine.

I'm trying to keep the core code from being bloated, and am very hesitant on adding new features/requests which requires me to write a lot of new code.

If you want to disable the tags removal transition, you can set the CSS variable --tag-hide-transition to 0s

[agusterodin]

I agree that the "load original values" thing isn't an issue and makes sense.

The only issue is that feeding state back into the component literally doesn't work. No react library or wrapper I have ever used in my years of react development has the sort of deficiency I describe below.

Consider this example:

const [tags, setTags] = useState(["Kevin", "Box"])

return(
<div>
<div onClick={() => setTags([])>Clear All</div>
<Tags
      value={tags}
      onChange={(event) => {
          setTags(["Some value!!!"]) 
      }}
    />
</div>
)

The desirable and proper react behavior for this would be that regardless of what tag you attempt to create/delete/change, the form would not make the change but instead immediately set the tag input to show a tag that says "Some value!!!".

[yairEO]

ok ok no need to get angry, I understand you want a "controlled" component, let me think how can this be done.

But can you at least explain to me why do you want it to behave like that? in what way does it not work for you well currently?

[agusterodin]

My bad!! I didn't mean to come off as rude.

Having the onChanged event set the react state which feeds back into 'value' prop causes a double firing of events (namely when you press backspace to delete a tag). You also get a firing of the onchange event when initializing if you provide a 'value' prop.

I also get a react warning saying "can not flush updates while react is already rendering" every time a tag changes.

In general, the reason controlled behavior is so important is we have a very complex react + redux application and the tag input needs to be properly synced with single source of truth. Updating the tags needs to be done bidirectionally (sometimes through input itself, some times changed by redux). It makes it really hard to do this if library is not well suited towards "controlled" behavior.

On a side note: It would also be a very nice convenience if the callback would provide the tag list as an array as opposed to having the user use event.target.value to get a string of the array which then needs to be converted into an array using something like JSON.stringify. Giving it directly as an array would lend itself very well to being a controlled component.

As for approach, I'm a fan of how this library implements controlled behavior: https://github.com/prakhar1989/react-tags

[yairEO]

Tagify was built in javascript and not in React and in vanilla-world, there are no controlled-components, so the wrapper doesn't magically transforms Tagify to be controlled, it a complex thing I must work on internally.

That other lib you linked to, which you've mentioned you're a fan of doesn't have close to half the features Tagify has.

I will make it work, just need to think what would be the best way to tell the component it shouldn't add/remove/edit tags by itself, but only fire a "change" event with the new-to-be value and then it is up to you as the developer to update your state and re-feed the component with the new value.

This won't be as performant as letting tagify handle the DOM because it knows best only to update what's needed and not everything entirely every time there's a change, but if this is what you absolutely need..

In my opinion you don't need to be so strict about this and just give up and let Tagify do what it does and you just make sure you keep your state in sync and not the other-way around. What does it matter anyway... it's not like it could get out-of-sync.

You can always change the value of your tagify component and re-sync it to whatever state you wish. The React demo (code starts after line 55) shows the "state" being fed-back as a new value

[agusterodin]

That would be awesome

[agusterodin]

Thanks kind sir

[agusterodin]

I am familiar with difference between a library that was built in react vs. a library that is built in vanilla that uses a wrapper to interact with the underlying javascript object in response to changes in props.

I think similar to the library I mentioned (which admittedly does have a lot fewer features) would be to have

• onAdd callback to tell you what single tag that was just attempted to be added (but don't actually add the tag to the underlying vanilla javascript instance). The user will then use this information to determine how (or if) they want to modify the tag state (the thing that gets passed into the 'tags' prop) on their own. Once they modify this on their own it will be reflected via the wrapper logic using loadOriginalValues behind the scenes)

• similarly, onRemove callback to tell you the index of the single tag that was just attempted to be remove (but don't actually remove the tag to the underlying vanilla javascript instance). The user will then use this information to determine how (or if) they want to modify the tag state (the thing that gets passed into the 'tags' prop) on their own. Once they modify this on their own it will be reflected via the wrapper logic using loadOriginalValues behind the scenes)

• similarly, onChange callback to tell you the index of the single tag that was just attempted to be changed along with the new value that it was attempted to be changed to (but don't actually change the tag to the underlying vanilla javascript instance). The user will then use this information to determine how (or if) they want to modify the tag state (the thing that gets passed into the 'tags' prop) on their own. Once they modify this on their own it will be reflected via the wrapper logic using loadOriginalValues behind the scenes)

The issue I see with just providing the all values via the onChanged method versus providing separate onAdd/onRemove/onChanged is that the person that is going to mutate the state themselves would likely want to know exactly how the input should change vs. just knowing what it would change to if tagify instance were to do its stock behavior. My particular use case would greatly benefit from onAdd/onRemove versus just using onChange.

I understand that the 'controlled' behavior would potentially big changes to your core tagify library and the stuff I am saying is a lot easier said than done. The best way I see this being done is having an "option" for controlled mode that will get passed in as an extra when we initialize tagify from our react wrapper (this thing: new Tagify(inputRef.current, { callbacks, ...options } || {})). This will inform the core library that the instance of tagify that we just created will be self managed.

As you mentioned, loadOriginalValues is not bad at all! The way native React libraries mutate the DOM vs. the way loadOriginalValues just recreates all of the DOM elements is completely unnoticable as far as performance goes in this scenario of not :) I fully understand that the only way to get this mutating behavior would be to rewrite the library in pure react which is completely impractical and for virtually no benefit.

[agusterodin]

As far as a use case for controlled behavior vs uncontrolled behavior in the context of using this library:

import React, { useRef } from 'react'
import { connect } from 'react-redux'
import Tags from '../Shared/TagifyWrapper'
import { setTags } from 'Redux/State/upload'
import { setToast } from 'Redux/State/general'
import { usePrevious } from 'Components/Shared/Hooks'
import { TAG_LENGTH_CRITERIA_ERROR } from 'errorHandling'
import { TAG_COUNT_CRITERIA_ERROR, TAG_UNIQUE_CRITERIA_ERROR } from '../../errorHandling'

const mapStateToProps = state => ({ uploadedImage: state.upload.uploadedImage })

const mapDispatchToProps = { setTags, setToast }

const TagifyInput = ({ uploadedImage, setTags, setToast }) => {
  const NO_TAG_PLACEHOLDER = 'Tags (Space Seperated)'
  const TAG_PLACEHOLDER = 'Tag'

  const tagifyRef = useRef()
  const previousImage = usePrevious(uploadedImage)

  const addHandler = event => {
    deleteOverlyLongTag(event)
    deleteExcessTag(event)
    deleteDuplicateTag(event)
    updateTagState()
    updatePlaceholder()
  }

  const editHandler = event => {
    deleteOverlyLongTag(event)
    updateTagState()
  }

  const removeHandler = event => {
    updatePlaceholder()
    updateTagState()
  }

  const updateTagState = () => {
    setTags(
      tagifyRef.current.value.map(current => {
        return current.value
      })
    )
  }

  const updatePlaceholder = () => {
    const tags = tagifyRef.current.value
    const textboxNode = tagifyRef.current.DOM.input
    if (tags.length === 0) {
      textboxNode.setAttribute('data-placeholder', NO_TAG_PLACEHOLDER)
      textboxNode.classList.remove('fit-width')
    }
    else {
      textboxNode.setAttribute('data-placeholder', TAG_PLACEHOLDER)
      textboxNode.classList.add('fit-width')
    }
  }

  const deleteOverlyLongTag = event => {
    if (event.detail.data.value.length > 15) {
      tagifyRef.current.removeTag(event.detail.tag)
      setToast({ text: TAG_LENGTH_CRITERIA_ERROR, style: 'error' })
    }
  }

  const deleteExcessTag = event => {
    if (event.detail.index > 4) {
      tagifyRef.current.removeTag(event.detail.tag)
      setToast({ text: TAG_COUNT_CRITERIA_ERROR, style: 'error' })
    }
  }
  const deleteDuplicateTag = event => {
    const duplicateTags = tagifyRef.current.value.filter((current, index) => {
      return current.value === event.detail.data.value && index !== event.detail.index
    })
    if (duplicateTags.length !== 0) {
      tagifyRef.current.removeTag(event.detail.tag)
      setToast({ text: TAG_UNIQUE_CRITERIA_ERROR, style: 'error' })
    }
  }

  // Remove all tags if image gets changed (ie form gets entirely reset)
  if (previousImage && previousImage !== uploadedImage) {
    tagifyRef.current.removeAllTags()
    updatePlaceholder()
  }

  return (
    <Tags
      tagifyRef={tagifyRef}
      className="form-textbox"
      delimiters=" "
      placeholder={NO_TAG_PLACEHOLDER}
      duplicates={true}
      addTagOnBlur={false}
      editTags={false}
      onAdd={addHandler}
      onEdit={editHandler}
      onRemove={removeHandler}
      onBlur={() => {
        tagifyRef.current.DOM.input.innerHTML = ''
        updatePlaceholder()
      }}
    />
  )
}

export default connect(mapStateToProps, mapDispatchToProps)(TagifyInput)

Pardon my bad code.

I want to display a toast when the user attempts to add a tag that goes beyond the limit or has too many characters. To achieve this, I leave the the maxTags property blank. With controlled behavior when a tag is attempted to get added I would be able to reject it (don't add to single source of truth) and show a toast. Ideally since I'm using redux the onAdd event would trigger an action which would then get received and processed by redux-saga. The saga would determine if the new entry that was attempted to get added wouldn't cause me to exceed the maximum number of tags and if it doesn't exceed the maximum number of characters allowed inside a tag. If too many tags I want to dispatch an action from redux to display a toast with a "too many tags" text, if tag is too long in character length I want to dispatch a toast with a "tags must be x length" message, or if tag successfully fits all criteria I want then to dispatch action to redux to add to the tags state array. Giving me just a list of all tags through the onChange event limits me into being able to distinguish where the tag was added. I understand that you have a maxCharacters and maxTag attribute but then using these puts you at the mercy of your core library behavior. Even worse, if you wanted to implement something even more custom (like say you only want to allow tags to contain certain characters), then you would be completely out of luck whereas having the information to make these decisions yourself and a controlled component would allow infinitely customizable behavior for react folks that need it with no need to add on to your core library.

Note that the wrapper could also benefit from being able to detect changes to the placeholder and apply them accordingly. This would allow me to pass placeholder={tags.length === 0 ? NO_TAG_PLACEHOLDER : TAG_PLACEHOLDER} and get rid of a lot of code where I do it manually. Would that be worth adding to the issue tracker as a separate issue?

Attached is the behavior that I try to achieve with the above code. Debatably toasts are not the best UX for this scenario but you see how this could apply to other people's use cases as well. It does work but could be much more streamlined if there was controlled behavior: https://streamable.com/gqqoyv

[yairEO]

Tagify now supports hooks, not React hooks, but "hooks" as the term was originally used before React or any other framework existed. Developers used to name certain placed in their code where other developers could intervene and chose how to proceed and in what manner.

I've only made a "beforeRemoveTag" hook, but the demo clearly shows how it can be used to stop Tagify from removing a tag.

I actually did code an "beforeAddTag" hook but have decided to delete it from the code because I couldn't think of a use-case scenario.

You should not forget tags may be editable, so you also need an "onBeforeEditDone" hook, so this means now you will need to listen to 3 hooks (add, remove, edit) to change manually sync the state and re-feed Tagify with new value (to force it to be "controlled").

Also, you must take into consideration that Taigy has other modes: "mix" & "select" for which you should also address when making a "controlled" component, because I do not want to release a version which only supports "regular" tags as "controlled".

Therefore I have concluded that developers should have a simple "channel" of communication with Tagify through onChange event listener, which I assumed already exists in their current tags (if they wish to move to using Tagify).

onChange listener is mode-agnostic and it's up to the developer to infer what was changed and update their state. For regular more, devs only need to sync the tagify.value Array collection, and for "mix" mode, devs should use the value of the textarea itself (which is "encoded" as stringified tags mixed with "normal" text)

Now, if you want a "controlled" component, then I think the best option is to pass this desire in Tagify's "settings", as a boolean, and this new setting will internally stop any DOM modification done by Tagify, but will fire a change event with the value that "should" have been rendered, and then the React developer could use that value to update their Store and re-feed it to tagify.

This is all in in theory, I'm not even sure it's fully feasible but I can try, when I get to it...

[yairEO]

I want to display a toast when the user attempts to add a tag that goes beyond the limit or has too many characters. To achieve this, I leave the the maxTags property blank. With controlled behavior when a tag is attempted to get added I would be able to reject it (don't add to single source of truth) and show a toast.

Tagify fires an invalid event which you can check in case a user did anything which isn't allowed by you. It tells you exactly the reason of the invalidation.

Invalid tags are only rendered but not added to the tagify.value array not to the original (hidden) input element.

tagify.on('invalid', console.log)

Ideally since I'm using redux the onAdd event would trigger an action which would then get received and processed by redux-saga. The saga would determine if the new entry that was attempted to get added wouldn't cause me to exceed the maximum number of tags and if it doesn't exceed the maximum number of characters allowed inside a tag.

As a UX/Product person, I do not like the idea of a user adding a tag and then wait an arbitrary amount of time before seeing it added. A server request might be slow and the user will be left in the dark, or worse, try to add the same tag again. Therefore the tag should be added ASAP and only then update the server with the new value, and in case of failure, remove that tag and notify the user something went wrong. You can also add a loader to the tag, to indicate something is going on behind-the-scenes:

tagify.on('add', ({detail:tag}) => {
	tagify.tagLoading(tag)
    ...do ajax...
    // if all good
    tagify.tagLoading(tag, false)
    // if server failed to update
    tagify.removeTags(tag)
    ...show a toaster message...
})

If too many tags I want to dispatch an action from redux to display a toast with a "too many tags" text, if tag is too long in character length I want to dispatch a toast with a "tags must be x length" message, or if tag successfully fits all criteria I want then to dispatch action to redux to add to the tags state array. Giving me just a list of all tags through the onChange event limits me into being able to distinguish where the tag was added. I understand that you have a maxCharacters and maxTag attribute but then using these puts you at the mercy of your core library behavior. Even worse, if you wanted to implement something even more custom (like say you only want to allow tags to contain certain characters), then you would be completely out of luck whereas having the information to make these decisions yourself and a controlled component would allow infinitely customizable behavior for react folks that need it with no need to add on to your core library.

Tagify doesn't have maxCharacters, and it's actually extremely powerful in what rules you can apply to anything without ever modifying the source code. You have pattern for regex tag validations for starters, which gives a ton of control. you also have the ability to transformTag not to mention many other settings..for every possible scenario.

Please I want to know a scenario which tagify cannot handle, please let me know of any.

[agusterodin]

Tying into your hooks is a pretty solid approach. If you had beforeAdd, beforeEdit, and beforeDelete hooks you could have it so the react wrapper exposes these hooks as -> onAdd, onEdit, and onDelete. The wrapper would trigger the passed in onAdd, onEdit, or onDelete callback the user provided while providing their callback function with all appropriate information like proposed value, index, etc etc, and always immediately provide the beforeX hook with a return value of false value so it doesn't actually add it to the dom elements directly from the underlying tagify instance. If this performs well in real life usage and there are no weird visual glitches it would achieve a very react-like controlled experience with minimal modification required to the code.

Also here is the flush updates warning I was talking about. It happens on your crazy tags demo:

In general I hear you loud and clear about your library having lots of built in functionality like invalid event, etc. I just think in general it would be cool to have full control over these tags and treat it as a 'dumb' component instead of using all the built in functionality though, especially since it lets you decouple some of your logic from this particular tag library and lets you test aforementioned custom logic very easily.

[yairEO]

I've also seen these can not flush updates while react is already rendering but am unable to pin-point their exact origin in the code. Now I am unable to reproduce a scenario which logs this error. weird.

[josejulio]

Now I am unable to reproduce a scenario which logs this error. weird.

FWIW I was seeing those weird errors locally, but it looks like they are gone after upgrading from 3.9.3 to 3.20.3

[Spoonrad]

I'm using a standard controlled component (I think ;) ) implementation where the parent component passes values as props, and the child component uses <Tags, passing the props value and onChange. onChange is a callback to the parent, which changes its state.value and passes state.value as props to the child component.

This leads to infinite callbacks. Sandbox Just uncomment the lines in the Parent.jsx file

Is this related to this issue? I don't have as much React / front-end expertise as you guys so I haven't fully followed this discussion.

[josejulio]

@Spoonrad The problem that I see is that every time you change the value, a new value is passed to Child, and that triggers an onChange (which repeats).

I'm not exactly sure what would be the best way to handle, but one option would be to compare current value with what's received If it's the same, don't change the state.

Also, the "value" provided seems to be the same for every call (e.g. it mutates the value contents without creating a new one) so you have to do some depth comparison (not a shallow one).

This compare logic would be better the Child, so maybe if the value is the same, don't post an onChange. If it changes, I would suppose it would be good to create a new value out of the contents of the other. To allow shallow comparison to consumers this.props.onChange({...tagifyInstance.value}). This would require keeping an state on the Child to keep track of previous values.

[Spoonrad]

I use this implementation with my antd components, which are derived off react-components, so I was just a little bit confused about why this was happening.

It's not a very big deal, I get the initial values as props in Child, pass them to the Child's state in its constructor and the Parent's state gets updated when a change happens. It still passes the value as props but that is only used once, in the Child's constructor.

So it works, it's just not super recommended by React AFAIK.

[yairEO]

I can internally do a deep-compare in the React wrapper and only proceed if new state is different

1. React Tagify doesn't need to be a "controlled" component, although it can be, I cannot think of a scenario it should be controlled.

2. @Spoonrad - Why are you still writing old-style react instead of functional React? I would advise to not write in this manner and start using hooks, they have been the norm for 3 years already

[Spoonrad]

@yairEO I haven't found any particular benefit to using hooks, except the minor benefit that it avoids component injection but that's about it. I don't really see the point of learning & migrating a new syntax if it doesn't have any benefit, and there is no planned deprecation of standard components

Just looked at some articles and I can see why hooks are useful, but their usage doesn't really respond to a need currently. We'll look into it, thanks for the feedback. :)

[yairEO]

One benefit is others will be easily able to read your code. I used to work with React classes before hooks and now I don't remember much at all, after not seeing class components for over 3 years.

React Hooks are so much more elegant IMHO, I love them so so much.

Anyway, I've pushed a fix to the Tagify React wrapper so new values will be compared to current ones and only if different, should render the new ones

[Spoonrad]

I can definitely think of scenarios where you want the component to be controlled by the way. I'm trying to set a form up that contains an input with Tags and it's been a nightmare. I'm still stuck with infinite loops, even if the value props never changes. For some reason, adding an item + the onChange callback + the value props (which never changes) leads to the infinite loop. The only solution I can think of to solve my problem is to add a ref to my child component and for the parent to get the data through the ref instead of using onChange.

I thought that making sure to always pass the same value in props would fix the issue but nope :/

I've updated to 3.25 and no change. The sandbox example posted above doesn't work either with 3.25

[yairEO]

@Spoonrad - what are the steps I need to do in order to reproduce the issue?

Currently the onChange in Parent.jsx looks like this:

onChange = (value) => {
    // Uncommenting this
    // makes everything break
    // with infinite callbacks
    this.setState({
      value: value,
      xxx: 'c'
    });
  };

I'm adding/removing tags and don't experience any infinite loop

I can definitely think of scenarios where you want the component to be controlled by the way

I would be happy to hear.

Tagify handles its own DOM, it will add/remove/edit nodes by itself and all a developer needs to do is listen to the change event and, if wished, save the state on a remote server. Having a controlled component means that at some point, after Tagify has already modified the DOM and the changes have been rendered, the developer wished to manipulate the state in some manner and re-feed the new modified state back to Tagify

[Spoonrad]

Typically what I'm used to doing in a form is the implementation in the sandbox (sorry I just edited it so the loop can happen again). A parent maintains an object state and has form inputs as its children. It passes the state as props to the children, which then pass the changed value back to the parent, which changes its state.

I'm used to doing things this way and right now I'm struggling a little with Tagify because of the issues with onChange. I'm probably going to stop trying to use onChange and instead get the values by using a ref, but that's usually not a recommended implementation.

[agusterodin]

Same here. I did this because I wanted to use my own business logic. It seems like this library doesn't want you to use it in a controlled fashion because it really wants you to rely on its own business logic implementations

[yairEO]

@agusterodin - This is a vanilla library that has a simple React wrapper to easily integrate Tagify within JSX. Have you ever written a React wrapper to a vanilla library? There are practical limitations. It is not as you say, that I actively chose not to allow a controlled component, it is just not feasible to really be controlled all-the-way. Below is a link of how it behaves as a fake "controlled" when in fact the DOM is changed so fast that the user does not notice that it is not a controlled component. For most purposed it wouldn't matter.

I have used Tagify myself, in very large scale, highly complex, React systems, without any issue.

---

@Spoonrad - it is not Tagify's fault here but your own code. You are setting the state in a way which is forbidden in React - mutating it and not creating a completely new Object on every state change, this is why Tagify does not register the changes of the props:

onChange = (value) => {
    this.setState({
      value: value  // never do this
    });
  };

You are simply pointing the state back to Tagify's internal state. React cannot compare the previous state with the new one in this way, since the previous once also was point to the *exact same Object! This is a crucial fundamental React thing right here.

Another problem with your code is that you did not use my latest version at all, but used some local file instead. See your import declaration for the Tags component - it's simply pointing to a local file in your demo, which is very much not the same as the latest in NPM:

import Tags from "./tagify/react.tagify"; // you use this
import Tags from "@yaireo/tagify/dist/react.tagify"; // when you need to use this

There are many other issues with your code, I had it simplified and here, everything works, and the component is in a controlled manner.

Here's the demo: https://codesandbox.io/s/tagify-react-wrapper-forked-0ggbx?file=/src/Child.jsx