Skip to content
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

Event types colliding with Object prototype properties don't work #3256

Closed
gibson042 opened this issue Aug 1, 2016 · 9 comments · Fixed by #4603
Closed

Event types colliding with Object prototype properties don't work #3256

gibson042 opened this issue Aug 1, 2016 · 9 comments · Fixed by #4603

Comments

@gibson042
Copy link
Member

@gibson042 gibson042 commented Aug 1, 2016

We use event.type as a key to get the array of event handlers, which throws an exception when we instead receive an Object.prototype property and treat it like an array: https://jsfiddle.net/9t4kqaqr/

Our standard fix is appending a space (cf. delegated selector match tracking and Sizzle createCache), but we should also consider Map for such purposes (ponyfilling as needed), especially if Data.js can be updated accordingly—one of @rwaldron's original goals when creating it.

@gibson042 gibson042 added the Event label Aug 1, 2016
@mgol
Copy link
Member

@mgol mgol commented Aug 5, 2016

but we should also consider Map for such purposes

Why not Object.create(null)? This one we wouldn't have to ponyfill.

@gibson042
Copy link
Member Author

@gibson042 gibson042 commented Aug 5, 2016

Why not Object.create(null)?

I thought we still supported environments where that fails, but things look good! 👍

@mgol
Copy link
Member

@mgol mgol commented Aug 5, 2016

Yup, and the changes required in that case will be smaller. :)

I'm only not sure if PhantomJS 1.x supports Object.create.

@timmywil timmywil added this to the 4.0.0 milestone Sep 26, 2016
@PetarMax
Copy link

@PetarMax PetarMax commented Dec 16, 2019

I've just rediscovered this bug. Is there a reason this hasn't been fixed yet?

@gibson042
Copy link
Member Author

@gibson042 gibson042 commented Dec 23, 2019

No one has yet gotten around to it. But a pull request would be welcome!

@timmywil
Copy link
Member

@timmywil timmywil commented Dec 25, 2019

I think we can implement the easy fix with spaces immediately, then consider using Map in 4.0.

mgol added a commit to mgol/jquery that referenced this issue Feb 5, 2020
…perties

Make sure events & data keys matching Object.prototype properties work.
A separate fix for such events on cloned elements was added as well.

The commit also removes two unnecessary checks:
1. A check for `events` in private data in `.trigger()` - only the `handle`
   key is needed.
2. A fallback of the `events` private data to an empty object in
   `jQuery.event.dispatch` because that method is called from the `handle`
   function in element's private data and both `events` & `handle` are set and
   removed at the same time.

Fixes jquerygh-3256
@mgol
Copy link
Member

@mgol mgol commented Feb 5, 2020

PR: #4603. I also fixed similar issues in manipulation & data modules using Object.create(null).

@mgol mgol removed this from the 4.0.0 milestone Feb 5, 2020
@mgol mgol added this to the 3.5.0 milestone Feb 5, 2020
mgol added a commit to mgol/jquery that referenced this issue Feb 10, 2020
…perties

Make sure events & data keys matching Object.prototype properties work.
A separate fix for such events on cloned elements was added as well.

The commit also removes two unnecessary checks:
1. A check for `events` in private data in `.trigger()` - only the `handle`
   key is needed.
2. A fallback of the `events` private data to an empty object in
   `jQuery.event.dispatch` because that method is called from the `handle`
   function in element's private data and both `events` & `handle` are set and
   removed at the same time.

Fixes jquerygh-3256
mgol added a commit to mgol/jquery that referenced this issue Mar 1, 2020
…perties

Make sure events & data keys matching Object.prototype properties work.
A separate fix for such events on cloned elements was added as well.

The commit also removes a fallback of the `events` private data to an empty
object in `jQuery.event.dispatch` because that method is called from the
`handle` function in element's private data and both `events` & `handle` are
set and removed at the same time.

Fixes jquerygh-3256
mgol added a commit to mgol/jquery that referenced this issue Mar 1, 2020
…perties

Make sure events & data keys matching Object.prototype properties work.
A separate fix for such events on cloned elements was added as well.

Fixes jquerygh-3256
mgol added a commit to mgol/jquery that referenced this issue Mar 2, 2020
…perties

Make sure events & data keys matching Object.prototype properties work.
A separate fix for such events on cloned elements was added as well.

Fixes jquerygh-3256
@mgol mgol closed this as completed in #4603 Mar 2, 2020
mgol added a commit that referenced this issue Mar 2, 2020
Make sure events & data keys matching Object.prototype properties work.
A separate fix for such events on cloned elements was added as well.

Fixes gh-3256
Closes gh-4603
mgol added a commit that referenced this issue Mar 2, 2020
Make sure events & data keys matching Object.prototype properties work.
A separate fix for such events on cloned elements was added as well.

Fixes gh-3256
Closes gh-4603

(cherry picked from commit 9d76c0b)
@rjaros
Copy link

@rjaros rjaros commented Apr 11, 2020

In my opinion this is a breaking change that should not go into 3.5.0 minor upgrade version. It can break a lot of dependent projects, like this one: snapappointments/bootstrap-select#2430

@mgol
Copy link
Member

@mgol mgol commented Apr 11, 2020

@rjaros Please submit a new issue with details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

5 participants