fix: Data.js must not overwrite exising element instance

[GeoSot]

Description

Bootstrap's Instances registry, by design, doesn't support multiple instances per element. However, in the rare case we are trying to re-initiate the same component for second time, it wrongly overrides the already existing instance
Details at #37245

Motivation & Context

Accidentally wrong usage of components constructor (ex:new Toast), may lead to a variety of runtime errors and side effects that are not expected

Type of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Refactoring (non-breaking change)
• Breaking change (fix or feature that would change existing functionality)

Checklist

• I have read the contributing guidelines
• My code follows the code style of the project (using npm run lint)
• My change introduces changes to the documentation
• I have updated the documentation accordingly
• I have added tests to cover my changes
• All new and existing tests passed

Related issues

closes #37245
rel: #37500

[julien-deramond]

I haven't checked the details of the PR but are we supposed to have this random behavior with a lot of errors displayed in the console while running the JS tests?

$ npm run js ; npm run js-test
.................
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.alert.'
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.alert.'
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
.........................................................
Chrome Headless 107.0.5304.110 (Mac OS 10.15.7): Executed 794 of 794 SUCCESS (23.895 secs / 23.604 secs)
TOTAL: 794 SUCCESS

$ npm run js ; npm run js-test
................................................................................
...................................
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.tooltip.'
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.tooltip.'
.............................................................
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.tab.'
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.tab.'
.................................................................
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.alert.'
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.alert.'
.........................................................
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.collapse.'
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.collapse.'
......................................
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.carousel.'
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.carousel.'
................................................................................
................................................................................
.......
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.modal.'
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.modal.'
..............................................................................
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.dropdown.'
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.dropdown.'
...............................................................................
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.toast.'
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.toast.'
......
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.button.'
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.button.'
................................................................
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.scrollspy.'
ERROR: 'Bootstrap doesn't allow more than one instance per element. Bound instance: bs.scrollspy.'
................................................................
Chrome Headless 107.0.5304.110 (Mac OS 10.15.7): Executed 794 of 794 SUCCESS (24.117 secs / 23.714 secs)
TOTAL: 794 SUCCESS

[GeoSot]

And it is fair to see these messages, as we have done things like this in the tests (ex: initialize two Alerts on the sαme element)

Any ideas, how we should handle it?

the other idea is something like this, but I feel it worse

[alpadev]

Since I'm the one to blame for the implementation, I wanted to put in my two cents.

With regards to the overwriting behavior questioned here #37245 (comment), this was in line with the prior spec:

bootstrap/js/tests/unit/dom/data.spec.js

Lines 30 to 44 in 088c727

	it('should change data if something is already stored', () => {
	fixtureEl.innerHTML = '<div></div>'
	const div = fixtureEl.querySelector('div')
	const data = {
	test: 'bsData'
	}
	Data.setData(div, 'test', data)
	data.test = 'bsData2'
	Data.setData(div, 'test', data)
	expect(div.bsKey).toBeDefined()
	})

---

About this PR. The fix is breaking the expectation imho (if i'm not mistaken) e.g.:

const instance1 = new Component(element);
const instance2 = new Component(element);

Component.getInstance(element) === instance1 // true
Component.getInstance(element) === instance2 // false

This could be considered a breaking change.

The second instance won't be set into the registry but it is still being initialized. As such, if someone is expecting getInstance() to return the actual instance for the element, this won't be the case.

---

My first proposal for the implementation was throwing an error but it was decided against doing it that way, in risk of breaking people's code (#32180 (comment)). On a side note - even with the fix or with the implementation that is in the main branch, there is the possibility to instantiate various components on the same element, even though only one is stored in the registry.

If we really would like to enforce some certain behavior, i'd rather propose to throw an error. That way the instantiation of another component would be cancelled, instead of just showing a message and preventing the registry to be updated.

Changing the PR to throw an error could be considered a breaking change, and won't really fix the issue mentioned in #37245. So another approach could be to dispose() a component if it already exists in the registry, prior updating it - instead of requiring a user to do it manually. Imho this should be a proper way of dealing with that issue.

@GeoSot @XhmikosR WDYT?

---

Update:

Pushed a draft PR with the solution mentioned here - see #37584

Another approach could be to always return an instance saved in the registry if someone tries to reinitialize a component that already was bound on an element. This could eventually render the getOrCreateInstance() method obsolete, as that would make it the default behavior. Not sure though how to deal with updating the config in that case. Any thoughts?

[GeoSot]

Thank you @alpadev for your answer on this.

I prefer your first approach (throw exception), but for sure would be a breaking change.
Maybe is ok for v5, to let it be on dev's responsibility, and on next major version (v6), start throwing exceptions

[GeoSot]

In case of same key, could just silently return, without overriding it