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

Embed JS: fix incompatibilities with sphinx 6.x (jquery removal) #9359

Merged
merged 7 commits into from
Aug 4, 2022

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Jun 21, 2022

Our theme still doesn't work becuase of readthedocs/sphinx_rtd_theme#1299.
But this PR doesn't depend on the theme, it can be merged as is.

ref sphinx-doc/sphinx#10574

@stsewd stsewd changed the title Emebed JS: fix compatibility with sphinx 6.x (jquery removal) Emebed JS: fix incompatibilities with sphinx 6.x (jquery removal) Jun 21, 2022
@stsewd stsewd changed the title Emebed JS: fix incompatibilities with sphinx 6.x (jquery removal) Embed JS: fix incompatibilities with sphinx 6.x (jquery removal) Jun 21, 2022
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

I think this code is fine. However, if I understand correctly, it seems there are some edge cases we won't be handling that I suppose will break in production due to the order of the scripts loaded.

readthedocs/core/static-src/core/js/doc-embed/search.js Outdated Show resolved Hide resolved
Comment on lines 29 to 30
// Inject JQuery if isn't present already.
if (!window.jQuery) {
Copy link
Member

Choose a reason for hiding this comment

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

What's up if this line is executed first and then jQuery is loaded by a different extension/theme/etc?

I'm thinking about this scenario;

  1. this code is run and we inject jQuery
  2. another .js file is loaded that includes jQuery
  3. once domReady is called, we will have 2 different jQuery versions/files loaded

Shouldn't we inject jQuery synchronously inside domReady so we wait for all the scripts to be loaded first and avoid this scenario?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is going to be a problem anywhere we try to inject jquery, since the code to be executed after jquery is done loading is inside an asynchronous callback (onload). I think the only difference would be that the page won't be blocked till jquery is done downloading.

@humitos humitos requested a review from agjohnson June 27, 2022 11:29
@mgeier
Copy link
Contributor

mgeier commented Jul 9, 2022

Is it really necessary to inject jQuery at this point?

Sphinx still has jQuery, it is only planned be removed in Sphinx 6.0: sphinx-doc/sphinx#10070

Note that https://www.sphinx-doc.org/ uses the master branch, with jQuery already removed, but they have added the JS frameworks to their docs already: https://github.com/sphinx-doc/sphinx/blob/master/doc/conf.py#L181-L186, so it shouldn't be necessary to add it from the RTD side.

@stsewd
Copy link
Member Author

stsewd commented Jul 9, 2022

Is it really necessary to inject jQuery at this point?

There are some themes on mkdocs that don't include jQuery (material).

@stsewd stsewd requested a review from a team as a code owner August 4, 2022 18:21
@stsewd
Copy link
Member Author

stsewd commented Aug 4, 2022

I have updated this PR to not inject jquery yet, so isn't blocked on that, this still removes some dependency on jquery.

Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

This is a good refactor, it makes a lot of sense to chip away at jQuery removal like this

@stsewd stsewd merged commit e2e055b into main Aug 4, 2022
@stsewd stsewd deleted the sphinx-jquery branch August 4, 2022 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants