Embedded JS: Conditionally inject jQuery #9861

stsewd · 2023-01-04T16:01:05Z

For injecting the flyout menu we are relying on a script from our theme, that script depends on jQuery, so it needs to be present for the flyout menu to work.

readthedocs.org/readthedocs/core/static-src/core/js/doc-embed/sphinx.js

Lines 43 to 59 in 67c44a1

    
           /// Inject the Read the Docs Sphinx theme code 
        
           /// This is necessary on older versions of the RTD theme (<0.4.0) 
        
           /// and on themes other then the RTD theme (used for the version menu) 
        
           if (window.SphinxRtdTheme === undefined) { 
        
               sphinx_theme = require('sphinx-rtd-theme');  // eslint-disable-line global-require 
        
               var theme = sphinx_theme.ThemeNav; 
        
               // Enable the version selector (flyout) menu 
        
               // This is necessary for 3rd party themes 
        
               domReady(function () { 
        
                   setTimeout(function () { 
        
                       if (!theme.navBar) { 
        
                           theme.enable(); 
        
                       } 
        
                   }, 1000); 
        
               });

We have migrated most of our code to vanilla JS,
but this piece isn't simple to migrate.

For injecting the flyout menu we are relying on a script from our theme, that script depends on jQuery, so it needs to be present for the flyout menu to work. https://github.com/readthedocs/readthedocs.org/blob/67c44a1f1b27dcc66bd14520d019459fcf86354d/readthedocs/core/static-src/core/js/doc-embed/sphinx.js#L43-L59 We have migrated most of our code to vanilla JS, but this piece isn't simple to migrate.

ericholscher

This is a good addition, and something I've been wanting for a while now 👍

readthedocs/core/static-src/core/js/readthedocs-doc-embed.js

ericholscher · 2023-01-04T16:35:37Z

readthedocs/core/static-src/core/js/readthedocs-doc-embed.js

+    console.log("JQuery not found. Injecting.");
+    var script = document.createElement("script");
+    script.type = "text/javascript";
+    script.src = "https://code.jquery.com/jquery-3.6.3.min.js";


Should we define this as a constant? Looks like we have 3.5.1 in current RTD docs: https://docs.readthedocs.io/en/stable/_static/jquery.js

Do you mean like something people can override? We would need to have two constants, one for the url and the other for the integrity hash. But if people want to inject a specific version, they can just do that, since we inject ours only if jquery isn't present.

Should this also be loaded from a readthedocs.io domain because of CSP?

Yeah agreed with @benjaoming, for CSP and CORS related headaches, this should be from our own domain.

I'd probably say we should just inject our existing jQuery vendored bundle, which is compiled using Gulp/Bower. Our current vendored jQuery is 2.0 however, but the API hasn't changed too much as well: https://github.com/readthedocs/readthedocs.org/blob/main/bower.json#L17

this is loaded from docs domains, we don't have a CSP there, but +1 on serving this from /_/static/. About the version, guess, anything that works with https://github.com/readthedocs/sphinx_rtd_theme/blob/0.3.1/js/theme.js should be fine. But since our theme was depending on the jquery version from sphinx not sure if there would be any problems, but running locally should be enough to test it I guess

I'm guessing we're also probably okay upgrading the jQuery version installed via Bower, we don't do anything particularly interesting in the existing templates/JS.

Unable to find a suitable version for jquery, please choose one by typing one of the numbers below: 1) jquery#~1.8.1 which resolved to 1.8.3 and is required by jquery-ui#1.8.23 2) jquery#3.6.3 which resolved to 3.6.3 and is required by readthedocs 3) jquery#>=1.5 which resolved to 3.6.3 and is required by jquery.payment#1.3.3

Don't think we are using the other plugins. I went ahead and wrote 2!, but I don't see the new files being copied to media, hmm, do we have a script for copying the files? 🤔

Traveled back in time to find npm run vendor

https://github.com/readthedocs/readthedocs.org/blob/3.11.0/docs/development/front-end.rst

I don't recall what we even use jquery-ui for, but the latest version of jquery-ui is a minor version bump and supports jQuery =< 3.6.

Good question on the static files. Vendored libraries are processed with Gulp here:
https://github.com/readthedocs/readthedocs.org/blob/main/gulpfile.js#L193

And are output here:
https://github.com/readthedocs/readthedocs.org/tree/main/readthedocs/static/vendor

As far as I know, this should just work with static collection. I see the assets in prod, and are requested from the application successfully.

Ah missed your reply until reload. I thought gulp build built all assets, but the command is separate, gulp vendor.

readthedocs/core/static-src/core/js/readthedocs-doc-embed.js

stsewd · 2023-01-04T22:32:19Z

readthedocs/core/static-src/core/js/readthedocs-doc-embed.js

+        // Set jQuery to its expected globals.
+        window.$ = require("jquery");
+        window.jQuery = window.$;


Not really sure if this is the way of doing it... but just adding the script from vendor/jquery.js doesn't add the global variables :/ /cc @agjohnson

Yup, this is normal. Relying on every module to be a global is an antipattern that doesn't work well with various forms of module importing -- commonJS, AMD, ES6 module imports, etc. Modern JS would tend to use jQuery as a module instead.

Your change is about what I'd do.

stsewd · 2023-01-04T23:53:47Z

Ok, I have updated this PR to:

Inject jquery from the same domain as the docs using our proxied static files endpoint.
Update the jquery version to the latest released one.
Updated all of our vendor js files with npm run vendor (we should probably update our docs/script to include it, looks like it was lost after the docker update Embedded JS: Conditionally inject jQuery #9861 (comment))
Did also test around our dashboard to see if the jquery update didn't break anything.
Ads and flyout menu work on https://test-builds.readthedocs.io/en/sphinx6.x-default/ with this change

stsewd · 2023-01-04T23:55:15Z

readthedocs/doc_builder/backends/mkdocs.py

+            "proxied_static_path": self.project.proxied_static_path,
+            "proxied_api_host": self.project.proxied_api_host,


We were missing including these for mkdocs, but it's only relevant when using a path different from _/, so

benjaoming

Solid work @santos22 💯 I'm wondering if there's any adjustments that we'd want to make in the Sphinx 6 post readthedocs/blog#201

On top of my head, I think it's great that this change ensures that nothing breaks - both our own Fly Out menu and whatever people have going on in their themes and projects can continue to use jQuery.

However, I'm a bit 👎 on documenting jQuery as something that's provided, hence also mentioning this in a blog post. People might use it, and then it gets even harder to remove.. when we want to deprecate it at some point.

I suppose that proxied_static_path is a heavily cached location?

stsewd · 2023-01-05T17:01:44Z

I suppose that proxied_static_path is a heavily cached location?

Yeah, that's cached at the CDN level.

Also, I think you tagged the wrong user p:

Solid work @\santos22

agjohnson

This is a great fix for now 👍

I'll definitely want to remove this dependency as we break apart the theme and flyout JS. There definitely shouldn't be theme specific JS in the flyout JS and there shouldn't be flyout specific JS in the theme.

I guess this is related to the jquery update from #9861, or maybe this line wasn't working and we were relying in something else? Not sure why I didn't notice this before, maybe my browser cache :( Ref #9861

Loading jQuery is not needed since this PR was merged in Read the Docs: readthedocs/readthedocs.org#9861. And exposing app.{info,warn,debug} was not needed since 2018: readthedocs/readthedocs-sphinx-ext#51.

Loading jQuery is not needed since January 2023: readthedocs/readthedocs.org#9861 And exposing app.{info,warn,debug} is not needed since September 2018: readthedocs/readthedocs-sphinx-ext#51

stsewd requested a review from a team as a code owner January 4, 2023 16:01

stsewd requested a review from agjohnson January 4, 2023 16:01

auto-assign bot assigned stsewd Jan 4, 2023

ericholscher approved these changes Jan 4, 2023

View reviewed changes

stsewd added 2 commits January 4, 2023 11:51

Updates from review

90bec6b

Use our own domain

7b4267e

stsewd requested a review from a team as a code owner January 4, 2023 19:40

stsewd requested a review from humitos January 4, 2023 19:40

Use jQuery from our vendor dir

2eaf67b

stsewd commented Jan 4, 2023

View reviewed changes

Linter

326742d

stsewd commented Jan 4, 2023

View reviewed changes

benjaoming approved these changes Jan 5, 2023

View reviewed changes

benjaoming changed the title ~~Embedded JS: Inject jQuery is isn't present~~ Embedded JS: Conditionally inject jQuery Jan 5, 2023

stsewd mentioned this pull request Jan 5, 2023

Mention/add npm run vendor in our frontend dev steps #9867

Closed

agjohnson approved these changes Jan 5, 2023

View reviewed changes

stsewd merged commit 7ed657e into main Jan 5, 2023

stsewd deleted the inject-jquery branch January 5, 2023 21:53

stsewd mentioned this pull request Jan 9, 2023

Explicitly set JQuery globals on main site #9877

Merged

humitos mentioned this pull request Jan 31, 2023

'$ is not defined' in readthedocs-doc-embed.js #9414

Closed

mitya57 mentioned this pull request Mar 19, 2023

Remove obsolete workarounds in doc/conf.py sphinx-doc/sphinx#11250

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Embedded JS: Conditionally inject jQuery #9861

Embedded JS: Conditionally inject jQuery #9861

stsewd commented Jan 4, 2023

ericholscher left a comment

ericholscher Jan 4, 2023

stsewd Jan 4, 2023

benjaoming Jan 4, 2023

agjohnson Jan 4, 2023

stsewd Jan 4, 2023

agjohnson Jan 4, 2023

stsewd Jan 4, 2023

stsewd Jan 4, 2023

agjohnson Jan 4, 2023 •

edited

Loading

agjohnson Jan 4, 2023

stsewd Jan 4, 2023

agjohnson Jan 4, 2023

stsewd commented Jan 4, 2023 •

edited

Loading

stsewd Jan 4, 2023

benjaoming left a comment •

edited

Loading

stsewd commented Jan 5, 2023

agjohnson left a comment

	/// Inject the Read the Docs Sphinx theme code
	/// This is necessary on older versions of the RTD theme (<0.4.0)
	/// and on themes other then the RTD theme (used for the version menu)
	if (window.SphinxRtdTheme === undefined) {
	sphinx_theme = require('sphinx-rtd-theme'); // eslint-disable-line global-require

	var theme = sphinx_theme.ThemeNav;

	// Enable the version selector (flyout) menu
	// This is necessary for 3rd party themes
	domReady(function () {
	setTimeout(function () {
	if (!theme.navBar) {
	theme.enable();
	}
	}, 1000);
	});

		"proxied_static_path": self.project.proxied_static_path,
		"proxied_api_host": self.project.proxied_api_host,

Embedded JS: Conditionally inject jQuery #9861

Embedded JS: Conditionally inject jQuery #9861

Conversation

stsewd commented Jan 4, 2023

ericholscher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agjohnson Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd commented Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

benjaoming left a comment • edited Loading

Choose a reason for hiding this comment

stsewd commented Jan 5, 2023

agjohnson left a comment

Choose a reason for hiding this comment

agjohnson Jan 4, 2023 •

edited

Loading

stsewd commented Jan 4, 2023 •

edited

Loading

benjaoming left a comment •

edited

Loading