Include jquery in the theme #1299
Conversation
stsewd
force-pushed
the
avoid-js
branch
3 times, most recently
from
fc7ee58
to
46d99b2
Compare
Sphinx is removing jquery on the 6.x version. Our theme depends a lot on jquery.
stsewd
changed the title
src/theme.js
Outdated
| console.log("JQuery not found. Injecting."); | ||
| var script = document.createElement("script"); | ||
| script.type = 'text/javascript'; | ||
| script.src = "https://ajax.googleapis.com/ajax/libs/jquery/3.6.0/jquery.min.js"; |
There was a problem hiding this comment.
I probably wouldn't refer to a Google hosted file here, and instead would bundle a jQuery release with the theme.
There was a problem hiding this comment.
So, we just bundle jquery with the theme instead of conditionally injecting it?
There was a problem hiding this comment.
@ericholscher was +1 on injecting it conditionally.
There was a problem hiding this comment.
I think it's good to use CDN. However, I'd point it to the official one instead of Google, as I did in sphinx-hoverxref: https://github.com/readthedocs/sphinx-hoverxref/pull/204/files#diff-702aaa7a7fb4adbe9dd9ce74f658c943aa3321b24c88d959d5a838761bb6c902R331
Official docs: https://jquery.com/download/#using-jquery-with-a-cdn
There was a problem hiding this comment.
So, we just bundle jquery with the theme instead of conditionally injecting it?
Yup. The concerns here are privacy, security, and creating a dependency downstream on a third party service. Package maintainers likely need the file locally as well.
The historical benefits of hosting JS directly from a CDN are no longer all relevant, and probably don't outweigh the privacy/security negatives this brings:
There was a problem hiding this comment.
Changed to bundle jquery with the theme
src/theme.js
Outdated
| * | ||
| * @param {function} fn - Function to be executed after jquery has been loaded. | ||
| */ | ||
| function injectJQuery(fn) { |
There was a problem hiding this comment.
Webpack does this natively. We use this import method in other places for similar conditional imports.
There was a problem hiding this comment.
Didn't find any other places where the import method is used in our theme.
| $(function () { | ||
| $("[data-toggle='rst-debug-badge']").on("click", function () { | ||
| // Add debug actions to flyout menu. | ||
| document.addEventListener("DOMContentLoaded", function () { |
There was a problem hiding this comment.
Shouldn't we create a domReady() helper function as we are doing in the community repository?
There was a problem hiding this comment.
seems simpler to just use the event listener? If you mean to try to load jquery here as well, there is no need, since jquery isn't used till you press the "toggle" button, at that point jquery is already included/loaded.
There was a problem hiding this comment.
I'm just saying that instead of repeating over and over the document.addEventListener("DOMContentLoaded", function () { part we could create a helper function and reuse on the whole file.
src/theme.js
Outdated
| console.log("JQuery not found. Injecting."); | ||
| var script = document.createElement("script"); | ||
| script.type = 'text/javascript'; | ||
| script.src = "https://ajax.googleapis.com/ajax/libs/jquery/3.6.0/jquery.min.js"; |
There was a problem hiding this comment.
I think it's good to use CDN. However, I'd point it to the official one instead of Google, as I did in sphinx-hoverxref: https://github.com/readthedocs/sphinx-hoverxref/pull/204/files#diff-702aaa7a7fb4adbe9dd9ce74f658c943aa3321b24c88d959d5a838761bb6c902R331
Official docs: https://jquery.com/download/#using-jquery-with-a-cdn
|
This PR fixes an important issue, but I'm not including it for the 1.1 release, since there are alternative plans to depend on a new project sphinx-jquery and ship jquery in this extension. These plans should be released in the 2.0 series soon enough that it's before Sphinx 6 is released! |
Sphinx is removing jquery on the 6.x version.
I have avoided using jquery before the main call to the theme,
and on the main call from the theme load jquery if isn't present.
I have removed the require statement, since the jquery object is global,
and could not be defined at the start of the script.
Related: #1253