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

Defer loading JS #847

Closed
wants to merge 1 commit into from
Closed

Conversation

jacobtomlinson
Copy link

We use this theme as the base for our custom Sphinx theme in the Dask project. We've noticed some render flicker when we load the page and form looking at the profiler this seems to be due to JavaScript resources being fetched and executed before the style sheets.

This PR adds the defer attribute to the script resources which will fetch them asynchronously and execute them once the rest of the page has finished loading. Alternatively the script tags could be moved to below other DOM resources.

@jessetan
Copy link
Contributor

Is your issue solved by #756?

@jacobtomlinson
Copy link
Author

That would help with the symptoms, but doesn't resolve the underlying issue.

Removing modernizr will make this issue less noticeable, but it will still exist on poor network connections and if child themes decide to include other large JavaScript resources through the script_files template variable.

Loading and executing JavaScript in the head, especially before CSS, is going to give users a worse experience generally.

Copy link
Collaborator

@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.

Thanks for the fix! I think this all makes sense, however I don't feel like I have a great grasp on what possible problems we could hit with this change. We might have to do some testing with this change before merge.

@agjohnson agjohnson added the Needed: replication Bug replication is required label Oct 24, 2019
@agjohnson
Copy link
Collaborator

I'll mark as needing some replication. I know this is an issue already however it might help to test this locally, with a RTD instance, where we might be introducing JS that could be incompatible with this change.

@jacobtomlinson
Copy link
Author

Yeah the only issue I can think of would be introducing other JS which depends on a script that has been deferred. It should also be deferred to ensure the correct loading order.

Let me know if there is anything I can do to help.

@jessetan
Copy link
Contributor

Unfortunately I don't think we can add defer to everything, some JS is not suitable for deferred loading.

The theme code can be deferredm but that bundles jQuery and we have had issues before when we moved jQuery out of the <head>, see #328. We had discussions on performance in #545.

Instead I think we should match what Sphinx does; when custom JS is added, it supports setting attributes (like defer) that end up in the HTML.
Docs: https://www.sphinx-doc.org/en/master/extdev/appapi.html#sphinx.application.Sphinx.add_js_file
Code: https://github.com/sphinx-doc/sphinx/blob/76e9f57c2e4457dec11d258435b7a6c6ec5de005/sphinx/themes/basic/layout.html#L92

Together with #756, that could solve most of the scripts in head. If not, then we may need a "put theme code + jQuery in head or not" config switch.

@jacobtomlinson
Copy link
Author

Yeah this was the issue I was talking about, if inline JavaScript depends on a library like jQuery then it can't be deferred like this.

Given the discussions in #545, which seem to roughly be "putting the JS in the head will be slow but more stable", it would be good to allow finer control with a config switch. That was a user can sacrifice the ability to use jQuery inline to get good performance.

@rdb
Copy link
Contributor

rdb commented Nov 5, 2019

The flicker issue seems mostly solved in my experiments in Firefox by just moving the CSS link tags above the JavaScript link tags. I filed PR #853 to address this.

@jacobtomlinson
Copy link
Author

Replaced by #853

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needed: replication Bug replication is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants