Fix `input.value undefined`

[cavasinf]

Fix error when using trim() on an undefined value.

I don't know if you guys accept optional chaining for browser compatibility.
eg:

input.value?.trim() ?? ''

So I've ended up using a simple ternary operator.

[nwalters512]

@cavasinf can you share a reproduction for the original issue you found? I'm struggling to see how value could be undefined here.

[cbrandtbuffalo]

I'm seeing the same issue. I'm trying to integrate tom-select into Request Tracker, so it's hard to create a minimal test case. You can see it in this branch, but you need to set up an environment to run RT, which is some work: https://github.com/bestpractical/rt/tree/6.0/add-tom-select

Inspecting the element generating the error, it looks like an input created by tom-select rather than RT itself.

'<div class="ts-wrapper selectpicker form-control single input-hidden full has-items"><div class="ts-control"><div data-value="" class="item" data-ts-item="">open (Unchanged)</div><input type="text" autocomplete="off" size="1" tabindex="0" role="combobox" aria-haspopup="listbox" aria-expanded="false" aria-controls="SelectStatus-ts-dropdown" id="SelectStatus-ts-control"></div><div class="ts-dropdown single" style="display: none;"><div role="listbox" tabindex="-1" class="ts-dropdown-content" id="SelectStatus-ts-dropdown"></div></div></div>'

It looks like a Status dropdown for a ticket, but the input doesn't look like what RT would generate.

Anything else I can provide to help debug?

[nwalters512]

If I had to hazard a guess, I'd say that for some reason this function is returning something unexpected:

https://github.com/orchidjs/tom-select/blob/78c0d4609ba29bf4ff45c86dff31234b14941419/src/vanilla.ts#L10-L27

The value returned from there is ultimately passed to getSettings:

https://github.com/orchidjs/tom-select/blob/78c0d4609ba29bf4ff45c86dff31234b14941419/src/tom-select.ts#L117

Which in turn is used in the init_textbox function that ultimately throws an error:

https://github.com/orchidjs/tom-select/blob/78c0d4609ba29bf4ff45c86dff31234b14941419/src/getSettings.ts#L145-L173

Consider adding a breakpoint, logpoint, or just a console.log to check what the return value of getDom is.

[cavasinf]

Hey, sorry for the late reply!

I honestly don’t remember exactly what scenario led us to encounter that issue.
It’s been three months, and that feels like ages ago for me! 😄

That said, if we expect both input and input.value to be defined in every possible case, why are we currently using the logical OR operator?
It seems like there might be some cases where this approach is necessary, but calling trim() on undefined or null will definitely throw an error.

[cbrandtbuffalo]

I updated my comment above to correctly display the element, it was hidden before. Looking at that input, it doesn't have a value, so that part makes sense. My thought was then to go add a value in the RT code. But looking at the input, it doesn't look like something generated by RT, but rather something tom-select created. It might be associated with an RT select, but there are a list of those "Status" update selects, and all have a value.

I agree with @cavasinf , looking at the code, the || '' suggests the code was intended to handle an empty value and it's just a bug.

[cavasinf]

Looking at the code before the JS vanilla refactoring:
https://github.com/orchidjs/tom-select/commit/56cefa2fe0ee5cdf3f095af8825083ffc119f556#diff-004d83612ab774e4911b1ba67da897c4205fceacd1700cdad66dedeb6a5fd514L24

The trim was encapsulating the OR operator.
This increasingly looks like an unintended BUG.

[nwalters512]

Looking at that input, it doesn't have a value, so that part makes sense.

All input elements have a value property in the DOM, even if there's no attribute in the markup itself. So I'm thinking you're providing a selector or an element that is neither a select nor an input. This is why I'm asking you for either an isolated reproduction or to place some breakpoints and see what's actually being passed around here!

My worry is that adding optional chaining here will just mask a deeper problem: either with your own code, or perhaps an invariant that tom-select should be asserting before this point.

I'd like to have a reproduction of this bug, ideally one that could be turned into a failing test case, before accepting a change.

[cbrandtbuffalo]

Working with this some more, I have an idea what is going on. I was using the code suggested in the docs to find all select elements on the page with a class:

document.querySelectorAll('.classname').forEach((el)=>{

On a dashboard in RT, the main page loads, then individual components load via AJAX. To cover all select elements, I was running an "initialize" function multiple times as components were loaded. I used this to avoid calling new on elements that had already been processed.

    if ( elt.tomselect ) {
        // This can get called on elements already initialized.
        return;
    }

As components are loading via AJAX, I think TomSelect was adding the dynamic input box. Even though my class wasn't directly on the ts-generated input tag, it looks like the TomSelect code added my classes to a surrounding div, along with "ts-wrapper". So I think that div was getting returned by querySelectorAll. And maybe that div is the element that doesn't have a value because it's not an input.

To test this, I tightened my selector to target only select.classname, no longer matching divs or inputs, and the console error is gone.

So I think users may see this error with applications running some AJAX that loads more markup dynamically, then they try to initialize again and end up grabbing some ts-generated elements that included the class name they used.

Does the above seem possible?

[nwalters512]

That definitely sounds plausible, yeah. Weird things will happen if you have a selector that targets any DOM elements created by tom-select. It seems like any classes specified on the element used in the TomSelect constructor will be copied to one of the elements that this library creates. You can probably use a selector like .classname:not(.ts-wrapper) to ensure that you don't target anything created by this library.

I'm inclined to say it's outside the scope of this project to check for and handle things like this. What do you think?

If this PR were merged, it would just mask the fact that you're sort of recursively creating TomSelect instances, so I don't think this particular change is a good idea.

[cbrandtbuffalo]

I'm inclined to say it's outside the scope of this project to check for and handle things like this. What do you think?

If this PR were merged, it would just mask the fact that you're sort of recursively creating TomSelect instances, so I don't think this particular change is a good idea.

Yeah, now that I see what is going on, I'm inclined to agree. The error is pointing out an unexpected use case. It would help with debugging to check the input for expected types and bail with a message, but that's a different PR.

I think a doc update would be easier and help new users. If I wanted to take a shot at providing some info on the two issues I encountered and possible fixes, where would that go? Maybe a new section at the bottom of the Usage page?