Bootstrap: Refactor data.js to use a `Map`

[XhmikosR]

It seems dom/data.js is the perfect candidate for using a Map instead of the custom implementation.

[alpadev]

@XhmikosR
I'd give myself a try on this, but I'm not sure if I fully understand the functionality.

From looking at the code, every element gets assigned an object within a bsKey property, that holds the information of some kind of unique-id and some key string to match against to. This key is used in the getData and removeData method. The unique-id is used as the key on the storeData object, with the data assigned that is passed to the setData function.

If one relies on the bsKey of that element, what exactly is the advantage of storing the data in the storeData, over storing the data on the element itself? Is this to hide things away?

Also if I understand it correctly, there can be only one {id, key} object at an elements bsKey at any time. Doesn't make this the key kind of unnecessary or is this so one doesn't get a wrong instance when the Component.getInstance() is called?

Currently the storeData looks something like this:

storeData: {
  1: TooltipInstance,
  2: ModalInstance,
  ...
}

How do you want the Map to look like and do you want me to get rid of the bsKey property?
Like Map<Id, Data> or like Map<Element, Data> or rather like Map<Element, Map<Key, Data>> or Map<Element, {Key:Data}>?

Do you prefer to use a WeakMap?

[Johann-S]

@alpadev here you have to replace storeData by a Map

[alpadev]

@Johann-S sorry my stupid questions, but I try to fully understand the intention behind this code.

The current implementation, as far as I get it, doesn't allow for multiple data on a single element at the same time, because setData will reuse the bsKey if it exists and override whatever data that is in storeData (without updating the key in that case). If you call setData on an element with different keys it will always override the information of the first used key, and getData will always return null for every other key aside the first one used.
That's the intended design?

Here is an example of an albeit fictional case that shows the implication of what I'm trying to say:
https://codepen.io/-alpa-/pen/KKMExwM

[alpadev]

Another question. Do I have to change only the src/data/dom.js and run js-test or are there any other steps required?

[rohit2sharma95]

The current implementation, as far as I get it, doesn't allow for multiple data on a single element at the same time, because setData will reuse the bsKey if it exists and override whatever data that is in storeData (without updating the key in that case).

@alpadev True 👍

If it is intended (Only a single instance can be associated with the element), then I think the element can be used directly in Map 🤔

const map = new Map();
map.set(element, instance);

[Johann-S]

yep it's intended currently we don't allow to use more than one plugin on a element