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

Breaking change to the data module in 3.5.0 #4665

Closed
rjaros opened this issue Apr 11, 2020 · 44 comments · Fixed by Holusion/holusion.com#67
Closed

Breaking change to the data module in 3.5.0 #4665

rjaros opened this issue Apr 11, 2020 · 44 comments · Fixed by Holusion/holusion.com#67

Comments

@rjaros
Copy link

@rjaros rjaros commented Apr 11, 2020

The change to the data module made here:

https://github.com/jquery/jquery/pull/4603/files#diff-38fa4ad21a97c2bf8d5b91d033df3335

breaks projects that depend on hasOwnProperty() function availability.

An example: snapappointments/bootstrap-select#2430
Many other project can be affected as well.

Such change should not be introduced as a minor version upgrade.

@mgol mgol added 3.x-only Data Discuss in Meeting labels Apr 11, 2020
@mgol mgol self-assigned this Apr 11, 2020
@mgol mgol added this to the 3.5.1 milestone Apr 11, 2020
@mgol
Copy link
Member

@mgol mgol commented Apr 11, 2020

Thanks for the report. I agree this is an unintended breaking change that we should fix in 3.5.1. Marking for team discussion.

Perhaps we can leave the current solution for jQuery 4.0.0 and in 3.5.1 apply original @gibson042’s suggestion of appending a space to event names before storing in the internal data storage. This solution can break code that relied on the private jQuery._data API to have real event names as event keys without any space at the end but since it’s a private API, this looks more acceptable to me.

Some possible consumer are web browsers, I think Firefox is using this API for jQuery event support in DevTools. If that’s still the case, do you know, @bzbarsky, who at Mozilla we could contact to coordinate any changes to this API?

@rjaros
Copy link
Author

@rjaros rjaros commented Apr 11, 2020

Any chance on quick 3.5.1 release?

@mgol
Copy link
Member

@mgol mgol commented Apr 11, 2020

@rjaros our release process is pretty involved and we still need to discuss & iron out the fix. Don’t expect it within days, more like a few weeks.

@mgol
Copy link
Member

@mgol mgol commented Apr 11, 2020

We’ll monitor the situation. If we get info about many more projects broken by this change, we’ll try to get the fix out quicker. No promises about specific timing, though: it’s Easter and we’re all volunteers.

@XhmikosR
Copy link

@XhmikosR XhmikosR commented Apr 11, 2020

@mgol it seems we are affected by the aforementioned change too: twbs/bootstrap#30553

Should I open another issue?

@mgol
Copy link
Member

@mgol mgol commented Apr 11, 2020

@XhmikosR no, let’s track all similar bugs in this issue.

@XhmikosR
Copy link

@XhmikosR XhmikosR commented Apr 11, 2020

So, basically the failures can be seen here where I updated jQuery to v3.5.0 and adapted our tests: CI errors

The code is quite old, since our v3.3.4.

@MartinHotmann
Copy link

@MartinHotmann MartinHotmann commented Apr 11, 2020

Indeed this is also affecting our system (InvoicePlane), jQuery 3.5.0 is tiggering many erros and also breaking the whole JS functonality. Indeed braking changes should not be implemented in a normal release, but instead in a mainrelease (^4.0.0) in jQuery as this will automatically break all projects which are based on jQuery and trust it to not include breaking changes within the same mainversion.

Also we had to change our package.json to not update to jQuery 3.5.0 and stay on ~3.4.1 for not running into this issue.

For us the eror was:
jQuery.Deferred exception: e.hasOwnProperty is not a function TypeError: e.hasOwnProperty is not a function

@dmethvin
Copy link
Member

@dmethvin dmethvin commented Apr 11, 2020

We appreciate the quick notification on this breakage and intend to release an update relatively soon but as @mgol said it's Easter weekend. Whatever you were doing on Friday morning before 3.5.0 was released, continue to do that until 3.5.1 is released.

@bzbarsky
Copy link

@bzbarsky bzbarsky commented Apr 11, 2020

@jasonLaster or @nchevobbe might know whether there are Mozilla dependencies here?

@mgol
Copy link
Member

@mgol mgol commented Apr 11, 2020

Thanks, Boris. @jasonLaster / @nchevobbe could you point me to the code that handles jQuery listeners in Firefox?

Same question to @paulirish; Chrome supports framework listeners and this could affect that support.

@nchevobbe
Copy link

@nchevobbe nchevobbe commented Apr 11, 2020

Hello @mgol
This is where we handle jQuery events: https://searchfox.org/mozilla-central/rev/8ed108064bf1c83e508208e069a90cffb4045977/devtools/server/actors/inspector/event-collector.js#406-409 .
Let us know of any changes so we can take that into account (although it would take a couple months for our changes to hit Firefox release).

@mgol
Copy link
Member

@mgol mgol commented Apr 12, 2020

I was thinking about it a bit more, my conclusions:

  1. I was focused on all the changes from commit 9d76c0b but the change to events should actually be fine. This is because the only way you can get ahold of it is via the private jQuery._data API which is private so if you use it, you're on your own. I'll note it's fine for web browsers to use it as it's just for use in DevTools; if it breaks, no website will be broken, the debugging experience will just be worse until the browser catches up.
  2. If we changed the events internal structure from a prototype-less object with regular event type keys to a regular object with keys with a space appended, it has a potential to break more code than the change to a prototype-less object. Although, from my testing it looks like both approaches work in both Firefox & Chrome.

Because of the above, I'd only revert the data change, deferring it to jQuery 4.0.0. PR incoming.

@mgol
Copy link
Member

@mgol mgol commented Apr 12, 2020

@XhmikosR Note that this fix won't fix all the other breakages with Bootstrap 3 due to the htmlPrefilter changes that we had to do as a security fix: https://jquery.com/upgrade-guide/3.5/

But at least for them there's a workaround for devs using Bootstrap.

@XhmikosR
Copy link

@XhmikosR XhmikosR commented Apr 13, 2020

@mgol are you sure our issue can be worked around?

The error that affects us all of our versions since v3.3.4 is because of the changes in #4603 (AFAICT) and Object.create which results in Cannot convert object to primitive value errors.

Here is the relevant WIP PR which fixes the issue https://github.com/twbs/bootstrap/pull/30559/files#diff-6ef28a975ef633bb3b3318b9917df9e5R338

@mgol
Copy link
Member

@mgol mgol commented Apr 13, 2020

@XhmikosR Not this issue, this issue we'll fix, most likely by reverting the change to src/data/Data.js from 9d76c0b.

The PR you linked to also applies updates for the changes to jQuery.htmlPrefilter and those will not be reverted so it's likely Bootstrap v3 code will remain incompatible with jQuery 3.5.0+ by default. The htmlPrefilter issue can be worked around by developers using Bootstrap 3 with jQuery 3.5.0+ via one of the steps outlined in https://jquery.com/upgrade-guide/3.5/ that restore jQuery.htmlPrefilter to it's pre-3.5.0 value.

@mgol mgol added the Blocker label Apr 13, 2020
@XhmikosR
Copy link

@XhmikosR XhmikosR commented Apr 13, 2020

The PR you linked to also applies updates for the changes to jQuery.htmlPrefilter and those will not be reverted so it's likely Bootstrap v3 code will remain incompatible with jQuery 3.5.0+ by default.

Ah, yeah those changes are only in our tests so people should read the release notes when it comes to their code.

I'm mostly concerned about the Object.create change since there are so many versions out there that are affected by that change, and unfortunately there's great fragmentation in versions people use (mostly 3.x which we no longer support and I'd hate to go back and release a new version).

Thanks for the help!

@boris-petrov
Copy link

@boris-petrov boris-petrov commented Apr 13, 2020

@XhmikosR - are we to understand that for Bootstrap 3 the only fix needed is the one for Object.create (which the jQuery team will fix) and not for jQuery.htmlPrefilter? That is, the workaround in jQuery's upgrade guide is not needed for Bootstrap 3 (because it affects only Bootstrap tests)?

@XhmikosR
Copy link

@XhmikosR XhmikosR commented Apr 13, 2020

@boris-petrov yeah, no other code seems to be affected from the other changes AFAICT; our CI passes.

@Johann-S please have another look in case I missed something.

mgol added a commit to mgol/jquery that referenced this issue Apr 13, 2020
The change in jquerygh-4603 made the object returned by `elem.data()` a prototype-less
object. That's a desired change to support keys colliding with Object.prototype
properties but it's also a breaking change so it has to wait for jQuery 4.0.0.

A 3.x-only test was added to avoid breaking it in the future on this branch.

Fixes jquerygh-4665
Ref jquerygh-4603
zachsvanhandel added a commit to zachsvanhandel/website-v1 that referenced this issue May 5, 2020
Version 3.5.0 of jquery introduced a [breaking change](jquery/jquery#4665) that was fixed in version 3.5.1.
@mgol
Copy link
Member

@mgol mgol commented May 5, 2020

jQuery 3.5.1 has been released: https://blog.jquery.com/2020/05/04/jquery-3-5-1-released-fixing-a-regression/

@0biWanKenobi
Copy link

@0biWanKenobi 0biWanKenobi commented May 5, 2020

It will sound like we're not letting you catch breath, and I hate to be that guy, but.. any ETA on a NuGet update? thanks for the hard work, guys :)

@mgol
Copy link
Member

@mgol mgol commented May 5, 2020

The jQuery team only maintains the jquery npm & Bower packages, the rest is done by the community. You need to ask the Nugget package maintainer to release the updated version.

@mryellow
Copy link

@mryellow mryellow commented May 7, 2020

I guess this is why linters say to use Object.prototype.hasOwnProperty.call(obj, 'foo')

gerrit-ovirt-org pushed a commit to oVirt/ovirt-engine-ui-extensions that referenced this issue Jul 6, 2020
Tooltips in HeatMaps were breaking because of jquery 3.5.0
bug jquery/jquery#4665.  Upgrading
to 3.5.1 resolves our tooltip issue.

Bug-Url: https://bugzilla.redhat.com/1853894
Change-Id: I730130af80158cb93c5eca5efa4a2160a77ccb1f
Signed-off-by: Scott J Dickerson <sdickers@redhat.com>
@ManuC84
Copy link

@ManuC84 ManuC84 commented Jul 13, 2020

I'm using these versions and I can't get the toggle "hamburger"(or any other collapse button) feature to work bootrstrap on laravel, any suggestions?

"devDependencies": {
"axios": "^0.19",
"bootstrap": "^4.5.0",
"cross-env": "^7.0",
"jquery": "^3.5.1",
"laravel-mix": "^5.0.1",
"lodash": "^4.17.13",
"popper.js": "^1.12",
"resolve-url-loader": "^2.3.1",
"sass": "^1.20.1",
"sass-loader": "^8.0.0",
"vue": "^2.5.17",
"vue-template-compiler": "^2.6.10"
}

@ctwillie
Copy link

@ctwillie ctwillie commented Jul 23, 2020

@ManuC84 Did you run npm install and npm run dev in your laravel project?

sdumetz added a commit to Holusion/holusion.com that referenced this issue Aug 21, 2020
…add source maps to files included from node_modules
sdumetz added a commit to Holusion/holusion.com that referenced this issue Aug 21, 2020
…add source maps to files included from node_modules
sdumetz added a commit to Holusion/holusion.com that referenced this issue Aug 21, 2020
…add source maps to files included from node_modules (#67)
sdumetz added a commit to Holusion/holusion.com that referenced this issue Oct 6, 2020
…add source maps to files included from node_modules (#67)
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.