-
-
Notifications
You must be signed in to change notification settings - Fork 78.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix interoperability issue regarding Event properties #36386
Conversation
Thank you, @alecpl, for your PR. Please take under consideration the need of below change (not sure if is needed), and it would be great to support your Pr with the proper tests function hydrateObj(obj, meta) {
for (const [key, value] of Object.entries(meta || {})) {
try {
obj[key] = value
} catch {
Object.defineProperty(obj, key, {
configurable: true,
get() {
return value
}
})
}
}
return obj
} |
@GeoSot I considered the change before, but I'm not sure it's really worth it. As for the tests, I'm afraid I will not be able to do that, as I have no idea about bootstrap's tests nor build system. |
I can help you witting the tests, but at least need a codepen that replicates the issue, you are trying to solve here |
The main issue is if you have an external library like cash-dom that is handling the same event as Bootstrap (e.g. a click handler on a Tab) and is setting the The second part of the patch ( Now thinking about this, and considering https://github.com/jquery/jquery/blob/5d5ea015114092c157311c4948f7cc3d8c8e7f8a/src/event.js#L306 we might indeed need the change you suggested. I mean it would be nice to be consistent inside Bootstrap and make all event properties read-only, but it may break compatibility with jQuery. I didn't test that. |
f061048
to
3266cb1
Compare
I did the change and added some tests to help. Just to note down here, I am not sure if I have understood the real issue on this situation, and if the proposed solution is valid. So if any other js dev have better knowledge, please d not hesitate to write an opinion |
d99859c
to
e2e45b8
Compare
987b889
to
e7707ce
Compare
- make possible to re-set read-only event properties - use hydrateObj() to set delegateTarget property Fixes twbs#36207
e7707ce
to
faf4519
Compare
@julien-deramond are we going to merge it? |
Fixes #36207