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

Upgrade jQuery to fix vulnerabilities #2239

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Conversation

elenatorro
Copy link
Contributor

@elenatorro elenatorro commented Nov 15, 2019

Related support issue: https://github.com/CartoDB/support/issues/2057

This PR fixes the dependency vulnerabilities in the current jQuery version. From @rjimenezda's comment:

  • jQuery is upgraded with all tests passing.
  • Some tests have been disabled for internal issues (it's commented on the tests), but it seems like the code they test works as expected.
  • We've changed clip-path-polygon library and we're using our own fork cause it also relies on jQuery fixed - using v0.1.14

Notes about the update in Builder:

  • All tests green doesn't really mean that it works 100%, we still have to test this against builder.
  • We need all libs to rely on the sale jQuery version, otherwise Builder won't build.
  • Currently seeing how builder looks, getting 58 broken tests right now.

jquery 3.4.1 respects default styles more, hence the 'block' => ''
Disabled some tests because of jasmine-ajax incompatibility
.load | .error => .on('load'|'error')
Copy link
Member

@Jesus89 Jesus89 left a comment

Choose a reason for hiding this comment

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

LGTM.

Note: I would remove the commented tests

@rjimenezda
Copy link
Contributor

I don't remember exactly what the issue was with the xdescribe one, so I'd keep that one just in case.

The commented one can burn tho

@csubira
Copy link
Contributor

csubira commented Jan 27, 2020

This PR also includes:
Bump eslint from 4.18.2 to 4.19.1 #2247
Bump mustache from 1.1.0 to 2.2.1 #2245 (Upgraded to 3.2.1 finally)
Bump jquery from 2.1.4 to 3.4.0 #2246 (Upgraded to 3.4.1 finally)
and it's related to https://github.com/CartoDB/product/issues/518

Copy link
Contributor Author

@elenatorro elenatorro left a comment

Choose a reason for hiding this comment

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

Nice PR!

As the issue description says, we've already run the jquery-migrate plugin to check if there's any deprecated method. Just in case, I took a look at https://api.jquery.com/category/deprecated/ and to the migration guide https://jquery.com/upgrade-guide/3.0/, and I think we could have missed a couple of things, mainly related with ajax and Promises in general:

$.Deferred (from the specs): https://github.com/CartoDB/carto.js/blob/jquery-upgrade/test/spec/core/model.spec.js#L58

We can keep using it like this in the model spec

$.ajax: done and fail should be then & catch now:
https://github.com/CartoDB/carto.js/blob/jquery-upgrade/src/geo/ui/legends/base/img-loader-view.js#L63

We can keep using done and fail as we're doing it right now

Also, about the dimension methods (width(), height()...) now they return undefined instead of null, and it might result into errors when calculating positions.

I would review these ones before continuing with the acceptance tests 👍

@elenatorro elenatorro self-assigned this Feb 4, 2020
@elenatorro elenatorro added the dependencies Pull requests that update a dependency file label Mar 3, 2020
@stale
Copy link

stale bot commented Jun 26, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants