refactor: use a Map instead of an Object in dom/data

[alpadev]

Fixes #32123

Preview: https://deploy-preview-32180--twbs-bootstrap.netlify.app/

[alpadev]

@XhmikosR I realised something in the dom/data spec https://github.com/twbs/bootstrap/blob/main/js/tests/unit/dom/data.spec.js#L30-L44

The test says 'should change data if something is already stored' but it expects the bsKey to be defined. Also if data.test is updated, there would be no need to call setData again as the object from the first call remains the same.

[rohit2sharma95]

The element can not be used directly here? 🤔
Ref: #32123 (comment)

My suggestions: alpadev#1

[Johann-S]

yep the element should be used here to the data

[alpadev]

Uhm that's a bit contradictory to this response #32123 (comment) but well

[Johann-S]

haha yes sorry @alpadev but things can change 🤷

[XhmikosR]

FYI this results in a nice size decrease (approx. 1.3-1.8% from the compressed gzip files) 🙂 https://ja2r7.app.goo.gl/5pUhLMyZD1iuZgYe9

I moved this to beta1 @Johann-S. Tests do seem to pass, although I notice a few of them are removed.

[rohit2sharma95]

Yes, we discussed the tests here: alpadev#1, though
Few of them are removed, I am not sure which tests should be added again 🙂

[XhmikosR]

BTW the DATA_KEY constants are only used in one place in each of the plugins except for popover.js and tooltip.js. The minifier already inlines those, so I guess we better keep them for consistency.

[Johann-S]

LGTM 👍 since we don't allow different instance on an element 👌

[XhmikosR]

@Johann-S My only concerns are:

1. if the removed tests should all be removed and if we still cover all cases we did before, like passing non-bs prefixed data attributes etc
2. I suppose if we want to support multiple keys in the future it shouldn't be a lot of work, right?

All in all, I love this change; it makes the code simpler and also saves us a good amount of code. I just want to make sure we aren't missing anything and that we won't be circling around in the future 🙂

[alpadev]

If it's expected to have multiple keys/instances on one element in a closer future, this might be not the ideal implementation to go with, I guess.

This basically removes the circuit of mapping an UID (and a DATA_KEY), that both been stored in an object at the elements' bsKey, to an instance that is saved in the mapData, and instead uses the 1:1 relationship of element and instance directly with the element being the key to the mapDatas' instance.

So before it was like:
A PluginInstance <> mapData.id <> element.bsKey.id mapping, with the DATA_KEY only being some kind of validation to check if Data.getData or Data.removeData is called with the same DATA_KEY, that has been stored in element.bsKey.key when Data.setData was called (for the first time).

Now it's like:
A PluginInstance <> mapData.element <> element mapping, with the DATA_KEY not being used here at all.

Another thing to mention, is that Plugin.getInstance(element) isn't "type-safe" anymore. Before if I had a Tooltip bound to an element, Tooltip.getInstance(element) would return some Tooltip-Instance, while Button.getInstance(element) returned null. Now every Plugin.getInstance returns whatever is saved in mapData for that element or null if nothing saved.

[XhmikosR]

Sounds like we need to rethink this. Let's keep this on hold for a few days until we get more feedback

…

On Sat, Nov 21, 2020, 19:45 alpadev ***@***.***> wrote: If it's expected to have multiple keys/instances on one element in a closer future, this might be not the ideal implementation to go with, I guess. This basically removes the circuit of mapping a UID (and an DATA_KEY), that both been stored in an object at the elements' bsKey, to an instance that is saved in the mapData, and instead uses the 1:1 relationship of element and instance directly with the element being the key to the mapDatas' instance. So before it was like: A PluginInstance <> mapData.id <> element.bsKey.id mapping, with the DATA_KEY only being some kind of validation to check if Data.getData or Data.removeData is called with the same DATA_KEY that has been stored in element.bsKey when Data.setData was called (the first time). Now it's like: A PluginInstance <> mapData.element <> element mapping, with the DATA_KEY not being used here at all. Another thing to mention, is that Plugin.getInstance(element) isn't "type-safe" anymore. Before if I had a Tooltip bound to an element, Tooltip.getInstance(element) would return some Tooltip-Instance, while Button.getInstance(element) returned null. Now every Plugin.getInstance returns whatever is saved in mapData for that element or null. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#32180 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACVLNNCPAEPZ42CF2D3JXDSQ74C7ANCNFSM4TZA7NYQ> .

[alpadev]

If I was to propose some implementation for the data, it should look something like this:

const elementMap = new Map(); // or WeakMap() even.. tbh lacking experience with WeakMaps

export default {
  set(element, key, instance) {
    if (!elementMap.has(element)) {
      elementMap.set(element, new Map());
    }

    const instanceMap = elementMap.get(element);

    // make it clear we only want one instance per element
    // can be removed at a later time when multiple key/instances are allowed
    if (!instanceMap.has(key) && instanceMap.size !== 0) {
      throw new Error(
        "Sorry, we currently allow only one instance per element."
      );
    }

    instanceMap.set(key, instance);
  },

  get(element, key) {
    if (elementMap.has(element)) {
      return elementMap.get(element).get(key) || null;
    }

    return null;
  },

  remove(element, key) {
    if (elementMap.has(element)) {
      const instanceMap = elementMap.get(element);

      instanceMap.delete(key);

      // free up element references if there are no instances left for an element
      // I guess this wouldn't be necessary with a WeakMap
      if (instanceMap.size === 0) {
        elementMap.delete(element);
      }
    }
  }
};

This uses both an element and a key to store some data/instance. It'd also allow for multiple key<>instance mappings, at a later point if it's decided to be necessary.

[XhmikosR]

@alpadev yup, that sounds like a better approach since that seems closer to what we have right now :)

[Johann-S]

@alpadev said :

Another thing to mention, is that Plugin.getInstance(element) isn't "type-safe" anymore. Before if I had a Tooltip bound to an element, Tooltip.getInstance(element) would return some Tooltip-Instance, while Button.getInstance(element) returned null. Now every Plugin.getInstance returns whatever is saved in mapData for that element or null if nothing saved.

The old behavior should be kept here, it's important to be type safe.

And I like your other implementation (#32180 (comment))

BTW when I created Data object, I tried to be as close as possible to how jQuery store data when we call .data on a jQuery object

needs more tweaks

[XhmikosR]

So, assuming we follow @alpadev's suggestion, would this still be considered a breaking change? If so, we need to get this done in beta1, otherwise we can move it to beta2.

[rohit2sharma95]

It is not a breaking change then IMO 🤔
Because as per the comment of @alpadev the old behaviour would be kept here.

[XhmikosR]

There are some last comments to address

[alpadev]

I'm not a big fan on console.logs in build files, but in this case I think it make sense to let people know why it doesn't work.

Personally I'm still against allowing people to have multiple plugin instances I think we'll have weird behavior and I don't see any valid use cases currently, that's all for my two cents 😄

I didn't mean to enforce my opinion here 🙂. My intention was just to explain my standpoint and to receive some input.

As I see it we have those options:

Allow the use of multiple components per element

• Just let it happen and have the implementers figure out problems that may occur
• Allow it but show a message (console.error/warn/log) to give them a hint <-- this is how it is for now

Disallow the use of multiple components

• Throw an error describing the problem
• Overwrite the first component but disposing it before (with or without a message)
• Overwrite the first component without cleaning up (with or without a message)
• Ignore/reject the second component (with or without a message) <-- this is how it is for now

There are some last comments to address

Yeah sorry.. I got my chance to try that thing called Covid and been totally mashed potato in my head for the last week or so. I'm on it.