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

Migrate jQuery (pt. 1) #9239

Merged
merged 7 commits into from
Nov 16, 2021
Merged

Migrate jQuery (pt. 1) #9239

merged 7 commits into from
Nov 16, 2021

Conversation

notfelineit
Copy link
Contributor

@notfelineit notfelineit commented Nov 15, 2021

Description

This PR addresses a request to upgrade jQuery to the latest version.

It completely removes jQuery dependencies from the vtgate and vttablet web UIs, as well as updates the plotly.js snippet in vtctld.

Surface Area

Orchestrator still needs to be looked at, but these UIs have been identified via this list.
Screen Shot 2021-11-15 at 4 40 55 PM

Main Changes

vtgate

It looks like jQuery in the vtgate UI was primarily used to mount Google's chart library. The chart library has been updated to use modern import and jQuery was swapped out here for a simple document.getElementById and the browser native fetch api. Confirmed that the charts still render and there is no jQuery anymore:
Screen Shot 2021-11-15 at 4 05 12 PM

vttablet

vttablet uses jQuery for the same reasons as vtgate, but in two places. Both have been updated to use document.getElementById and the fetch api. Confirmed that the charts still render and there is no jQuery:
Screen Shot 2021-11-15 at 5 10 54 PM

vtctld

vtctld does not actually import jQuery and it is not loaded on the vtctld dashboard. The only place that references jQuery is the old plotly.js snippet. I updated this snippet and confirmed the graph still looks the same:
Screen Shot 2021-11-12 at 9 55 10 AM

Related Issue(s)

N/A

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

No extra deployment steps necessary for these changes. They do not add any new libraries.

Signed-off-by: notfelineit <notfelineit@gmail.com>
Signed-off-by: notfelineit <notfelineit@gmail.com>
Signed-off-by: notfelineit <notfelineit@gmail.com>
Signed-off-by: notfelineit <notfelineit@gmail.com>
@notfelineit notfelineit changed the title Migrate jQuery Migrate jQuery (pt. 1) Nov 16, 2021
Signed-off-by: notfelineit <notfelineit@gmail.com>
Signed-off-by: notfelineit <notfelineit@gmail.com>
Signed-off-by: notfelineit <notfelineit@gmail.com>
@doeg
Copy link
Contributor

doeg commented Nov 16, 2021

Ah YAY great work (and great write-up)! 🎉 Is this ready for review? If so, I can look first thing tomorrow.

@notfelineit
Copy link
Contributor Author

@doeg Yes! Gotta get the tests to past though 😅

@doeg
Copy link
Contributor

doeg commented Nov 16, 2021

@notfelineit I can re-run the tests for you this morning. :') The failures are definitely unrelated to your PR.

The only test that needs attention is the one that checks for the release note PR labels here. (This seems like a good one to mention in the release notes, fwiw!)

Copy link
Contributor

@doeg doeg left a comment

Choose a reason for hiding this comment

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

YAYyy the best jQuery upgrade is one that removes jQuery entirely! 😈

Such great work! Updating these dusty UIs is no easy feat. :) I pulled your branch + ran it locally and everything looks good to me! Thanks again for such a thorough write-up with screenshots.

@@ -73,6 +73,8 @@ releases
/web/vtctld2/bower.json~
/web/vtctld2/public/bower_components/

# Local examples
/examples/local/vtdataroot
Copy link
Contributor

@doeg doeg Nov 16, 2021

Choose a reason for hiding this comment

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

Thanks for adding this! (I've always wondered why it's not been in the .gitignore all along.) :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problemo! :-)

Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

@doeg's review is the one that matters, but 👍
We can discuss which release(s) this actually needs to be backported to.

@deepthi deepthi merged commit ce61e56 into main Nov 16, 2021
@deepthi deepthi deleted the frances/plotly-replace branch November 16, 2021 18:08
@notfelineit notfelineit mentioned this pull request Nov 17, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants