Skip to content

Conversation

@ChristophWurst
Copy link
Member

As announced in the deprecation warning we will slowly upgrade jquery. This time to v2.2. A quick smoke test showed me a working files list and some basic settings, so the breakage might be minimal.

I would suggest to get this in as early as possible. So we notice the breaking changes in various apps and fix them before the 20 release.

👀 @nextcloud/javascript

@ChristophWurst
Copy link
Member Author

👀 @blizzz as our friend the ldap 🧙 might cause trouble

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews pending documentation This pull request needs an associated documentation update labels May 13, 2020
@MorrisJobke
Copy link
Member

👀 @blizzz as our friend the ldap 🧙 might cause trouble

Fully tested one LDAP wizard go from first to last page. Adding credentials, login details, users and groups worked just fine and there were no JS errors in the console (just a lot of deprecation warnings).

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and still works 👍

@e-alfred
Copy link

Do you have some kind of list of apps that still use JQuery and could potentially be affected by this?

@ChristophWurst
Copy link
Member Author

Do you have some kind of list of apps that still use JQuery and could potentially be affected by this?

No, this information is hard to acquire. Rule of thumb is the apps that still don't use webpack but have some UI to render

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst force-pushed the dependachristoph/npm_and_yarn/jquery-2.2.4 branch from 4ceea3b to 4ed6284 Compare May 14, 2020 09:53
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Seems to work still fine

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 14, 2020
@ChristophWurst ChristophWurst merged commit d6bb091 into master May 14, 2020
@ChristophWurst ChristophWurst deleted the dependachristoph/npm_and_yarn/jquery-2.2.4 branch May 14, 2020 12:26
@e-alfred
Copy link

Rule of thumb is the apps that still don't use webpack but have some UI to render

The JQuery warning comes up on pretty much every app at the moment, even the Files or Calendar etc., so it is not that helpful currently.

@ChristophWurst
Copy link
Member Author

I know. This is something we have to work on. For Files it's probably the most complex task to get this app into shape. But others should be easier.

@ChristophWurst
Copy link
Member Author

You can, for example, add https://github.com/nextcloud/eslint-config and it will automatically tell you which of the deprecated/removed globals are still used.

@juliusknorr
Copy link
Member

There actually seems to be a regression with this that causes the files app to fail loading (sometimes on firefox but more often in chromium)

image

It seems to be somehow related to the order how things are executed since the issue is mainly that the OCA.Files.FileList is not ready when the $(document).ready is being called.

image

Any idea @ChristophWurst ? I couldn't find anything helpful in the 2.2 changelog. 😞

@ChristophWurst
Copy link
Member Author

It seems to be somehow related to the order how things are executed since the issue is mainly that the OCA.Files.FileList is not ready when the $(document).ready is being called.

ah yeah. I do remember that this loading order issue arose when I attempted to upgrade to v2.2 in the past. Now it seemed resolved when I tested and I just assume the bundled scripts helped.

IIRC removing the defer from the scripts tags would solve the problem. The load event is handled differently in jQuery <2.2 and after.

@skjnldsv
Copy link
Member

Still an issue for me 🤷
Fix in #21921

@ChristophWurst
Copy link
Member Author

Documented at nextcloud/documentation#5160

@ChristophWurst ChristophWurst removed the pending documentation This pull request needs an associated documentation update label Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants