getOrCreateInstance and getInstance

[lekoala]

Would it be possible to add these two static methods on the base component ? I think it would be very beneficial for those who want to switch from bootstrap 5 and it only require a minor effort to implement these methods. What do you think?

[lekoala]

Tested quickly without updating anything else, this seems to work

import {getInstance as getBaseInstance} from 'shorter-js/src/misc/data';

  /** @static */
  static getInstance(element) {
    return getBaseInstance(element, this.name)
  }

  /** @static */
  static getOrCreateInstance(element, config = {}) {
    return this.getInstance(element) || new this(element, typeof config === 'object' ? config : null)
  }

and it would allow removing instance specific methods (like in the tabs component)

[thednp]

This change would require an entire sweep of changes to all V5 components and possibly more changes to the BaseComponent constructor, as getOrCreateInstance doesn't fit any purpose in the current state. Thoughts?

I believe this may help simplify components and potentially save space maybe.

[lekoala]

actually, I gave it a bit more thought, and initially, it can be as simple as adding the getOrCreateInstance since the getInstance method is already implemented in each class. so the first step can be quite simple. I really like the getOrCreateInstance function since it allows to deal with components without knowing in advance if it was already initialized or not somewhere else.

As for the getInstance method, It feels like a wasted opportunity to simplify the whole codebase since the getInstance should be available in each component and works in a consistent manner, but that can be done later since it doesn't bring any real benefit except a more consistent codebase.

[thednp]

To move getInstance into the BaseComponent static would create lots more strings and increase the size of all components.

EG

const self = getAlertInstance(myAlertElement);

would become

const self = BaseComponent.getInstance(myAlertElement);
// or this.getInstance(myAlert), but we don't know the value of `this` in many cases like event listeners attached to window/document

That multiplied by each method and each component would increase the overall library size, without a real benefit.

Now for the getOrCreateInstance method, again, no real benefit since our init code can already do that for all components at once, plus I prefer to dispose the components before re-init, to clear the garbage collectables, as well as update component instance options if changed. Thoughts?

[lekoala]

@thednp ok so maybe let's just limit to the getOrCreateInstance method. The goal here is to make the switch from bs5 seamless. I found it useful to get the instance from an element without knowing if it has been initialized already. in bs5, creating a new instance on an element already initialized doesn't work, therefore you have to use the getOrCreateInstance method. You don't always want to reinit/refresh the state of the component, simply get it based on an html node. It has almost no cost to add this extra method, no?

[thednp]

Our goal here isn't to have a perfect drop-in replacement for the original BS5 JavaScript, it wouldn't make sense in the first place. The reason why the original library works with later added elements is not because of getOrCreateInstance method, but because almost all events are attached to Document and delegated to component specific selector matches. This is something the original BS has inherited and maintained from jQuery, we didn't like that.

Back in the day we chose to attach events to component targets themselves to leave the global objects free from any listeners, which back in the day was a much needed performance boost.

But perhaps I didn't understand you?

[thednp]

@lekoala ?

[lekoala]

@thednp maybe it's me that's missing something. let's say i have an element with the tooltip and i want to enable or disable them. Here is some sample code where I use getOrCreateInstance

document.querySelectorAll('[data-bs-toggle="tooltip"].js-mobile-tooltip').forEach((el) => {
      let tooltip = bootstrap.Tooltip.getOrCreateInstance(el);
      // On large screen, display the badge
      if (w > MOBILE_SIZE && el.offsetWidth > 12) {
        tooltip.disable();
        el.removeAttribute("title");
      } else {
        // Enable tooltip for small screen or small items based on its content
        tooltip.enable();
      }
});

how is this supposed to work in bootstrap native? also, while i understand you don't aim to make a perfect drop-in replacement, in that case, exposing the same public api (getinstance, getorcreateinstance, dispose) helps. in this case, adding the most is almost free so I don't see any downside in doing it.

[thednp]

@lekoala

Well, you already have all the tools you need, why not use them already? How about this?

document.querySelectorAll('[data-bs-toggle="tooltip"].js-mobile-tooltip').forEach((el) => {
  let tooltip = BSN.Tooltip.getInstance(el) || BSN.Tooltip.init(el);
  // On large screen, display the badge
  if (w > MOBILE_SIZE && el.offsetWidth > 12) {
    tooltip.disable();
    el.removeAttribute("title");
  } else {
    // Enable tooltip for small screen or small items based on its content
    tooltip.enable();
  }
});

This completely invalidates the need for the getOrCreateInstance method.

Now since all components expose everything you need, including component specific selector, you can create your own builds to work with your own mass initialization script to enable your builds to handle jQuery style later added elements, turbolinks:load and whatever you want, all without a bit of unwanted or redundant code.

This is what BSN is really about: we don't want to lock users into a single option (or set of options) OR feature something that doesn't benefit the community at large.

Sweet?

[lekoala]

@thednp fair point, it's true that i can replicate the getOrCreateInstance myself using the getInstance || new pattern, however that feels like something that should belong to the library since I'm probably not the only one to do that. But I get your point and since you are the library owner, I fully respect your design decisions if you don't think that's necessary, i'll make the adjustements on my end, it's no big deal to add a helper method

[thednp]

Closing the issue due to no activity.