-
Notifications
You must be signed in to change notification settings - Fork 714
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
Added ADR #3 re: jQuery #2106
Merged
Added ADR #3 re: jQuery #2106
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
# Remove jQuery and jQuery UI dependencies | ||
|
||
📆 **Updated**: Aug 17, 2022 | ||
|
||
🙋🏽♀️ **Status** Proposed | ||
|
||
## ℹ️ Context | ||
|
||
- The current use of jQuery 1.7.1 is a low vulnerability (XSS) | ||
- The version is old and new changes are a bit of a pain (e.g. need to read old API docs) | ||
- The version can be upgraded but that takes effort (same as above, APIs have changed) | ||
- It's currently responsible for 80%+ (LOC) in `site.js` | ||
- We don't support legacy browsers, such as IE, anymore | ||
|
||
## 🤔 Decision | ||
|
||
Remove jQuery and jQuery UI dependencies. We will be using vanilla JavaScript, rather than immediately switching to another framework, to keep the performance overhead to a minimum. | ||
|
||
## 🎬 Consequences | ||
|
||
- Remove XSS vulnerability | ||
- Lighter pages | ||
- We can take advantage of everything the web platform had to offer without a layer of a polyfill-style library | ||
|
||
## Process | ||
|
||
We need to move each jQuery/jQueryUI piece one at a time and once we're done, delete the jQ\* code. A list of features we currently use (possibly incomplete): | ||
|
||
- dialogs | ||
- tabs | ||
- dragging and resizing | ||
- viewport offsets | ||
- sizing elements | ||
- getting OS scrollbar width | ||
- displaying local time | ||
- tooltips | ||
- scroll handling and animation | ||
- editting test labels via async requests (`$.ajax`) | ||
- sortable tables (Request Details) which is implementation #3 in addition to a new DIY one and one from Google JS APIs | ||
|
||
As prep work, move all jQ* dependencies out of `site.js` so it's easier to tell application callsites vs internal dependencies. Some of the jQ* code is not minified so it's not immediately obvious if this is application code. | ||
|
||
## 📝 Changelog | ||
|
||
- 07/07/2022 Proposed | ||
- 08/17/2022 Comments addressed |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the full list of dependencies just dialogs and tabs right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's some stuff in www/site.js
I saw a
$.ajax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a list of what I see in there: