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.

Sign up for GitHub

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

Jump to bottom

Avoid flashing and slow page loading by inlining style sheets and widgets #800

Open
wants to merge 4 commits into
base: gh-pages
Choose a base branch
from
Open

Avoid flashing and slow page loading by inlining style sheets and widgets #800

wants to merge 4 commits into from

Conversation

chrishtr
Copy link

Currently, bjc-r pages are very slow to load and have a lot of flashing-of-unstyled-content. This is because all of the
stylesheets, as well as some HTML content, is dynamically inserted by loader.js. Browsers will then do this:

Load initial HTML, and kick off request for loader.js
Render what they have (unstyled content, because there are no style sheets
loader.js loads and executes, which inserts styles sheets and widgets on the homepage
Browser kicks off requests for style sheets, and renders what it has (now with widgets, but still unstyled)
Style sheets load and the page renders its final, correct version
All of this can be avoided with a very simple edit - simply inline the style sheets and widgets in the page.

I've also made some tiny edits to llab so that it is forward-compatible with inlining. It was already compatible with it in most cases.

@chrishtr
Copy link
Author

I converted all of the HTML files to inline, by using a simple script.

Also, this PR is a redo of #797, which had some issues.

@chrishtr chrishtr mentioned this pull request Apr 16, 2021
Open
@chrishtr chrishtr changed the title Avoid flashing and slow page loading by inlining style sheets and widgetsnone Avoid flashing and slow page loading by inlining style sheets and widgets Apr 16, 2021
@chrishtr chrishtr mentioned this pull request Apr 16, 2021
Closed
@cycomachead
Copy link
Member

Thanks for the work, but the point of the system was that all of this content would not be duplicated.

@chrishtr
Copy link
Author

chrishtr commented Apr 16, 2021 •
edited
Loading

Thanks for the work, but the point of the system was that all of this content would not be duplicated.

Sure, but I think the poor performance and jumpiness during loading is much worse than some amount of code duplication in this case.

Do you have a suggested alternative that would achieve the same goals? I don't think it can be fixed with a runtime script
solution, in part because browsers only block rendering for parser-inserted style sheets, not script-inserted ones, and in part because extra round-trips are very slow on many networks. A runtime script could apply opacity:0 to the entire page until all resources are loaded, but that solves only the jumpiness problem, and not the
performance problem.

It could be fixed with a dynamic serving system such as PHP, or a build step that concatenates resources. This is typically
what more complex sites do.

Also, see bjc-edc/bjc-r#45. @brianharvey commented on an earlier version of that PR (bjc-edc/bjc-r#491) that the user experience improvements are worth achieving (my paraphrase of course).

@brianharvey
Copy link
Contributor

Yes and I still think so. It'd be best if we can have our cake and eat it too, getting fast loading without having to edit every curriculum page when we want to change the formatting or something.

@chrishtr
Copy link
Author

Hi, friendly ping on this PR.

Several points:

  1. This PR does not require inlining of widgets and CSS. If a new page is created that does not have them, then it will still work.

  2. Right now, all scripts other than loader.js are are loaded async and in a cascading manner. As far as I can tell so far, the only reason is that the scripts need to load in a particular order, and are also marked as async.

  3. I don't see any reason that the initial style sheets can't be inlined during eval of loader.js, e.g. by doing it directly from llab.preSetUp.

I could mitigate items 2 and 3 with one or more of the following approaches:

A. Inline style sheets directly from loader.js synchronously. This will avoid a delay if the page has inlined image resources.
B. Inline <link rel=preload src=[script]...> tags for all scripts synchronously from loader.js. This will reduce round-trip delays because it'll start the network fetches for these scripts earlier.
C. Change all scripts to be sync, and insert them synchronously from eval of loader.js. This will reduce the number of round-trips substantially.
D. Put opacity:0 on the body until everything has loaded, with a timeout in case it takes too long. This will avoid FOUC.
E. Synchronously initialize the injected widget HTML from loader.js, and move the script tag for loader.js into a child of <body>. This would involve moving code from curriculum.js to loader.js.

If you are willing to at least accept inlining the style sheets into the <head>, then option E would also suffice to avoid all FOUC.

If not, then at bare minimum I think these should be done:

  • A
  • B or C
  • D
  • E?

What do you think?

@cycomachead
Copy link
Member

Hi Chris,

Sorry I haven't had a chance to respond yet. I've got 4 courses and GitHub discussions are not really setup for something as intense as this. (3rd since PR comments aren't saved and get eaten...). I haven't taught CS10 in a very long time, though ostensibly I'm the one to still keep some of this running. But I will be in the Fall or Spring and I hope there's a good chance to make more significant changes then, too. Well, not in the middle of the semester, ideally.

But also, I do not think this is urgent. It hasn't been dealt with because, well, there's been 500 other things that have been a priority.

Sure, but I think the poor performance and jumpiness during loading is much worse than some amount of code duplication in this case.

In this case a single loading entry was basically the design goal ~7 years ago when this was created. Maybe it's a misguided goal, but I don't think code duplication is the answer. There are not that many updates that happen now and already there's a conflict. The flash annoying, but it has always been annoying.

It's good to know that the loader still works even when you don't include the necessary parts, though I think it's somewhat confusing then since you'd get into a 50/50 state. But I think this is something to consider.

That said, the loader itself is due to be burned in a fire, so I'm not defending it as a piece of engineering.

As far as general approaches, there are two that aren't mentioned -- one of these is likely what we'd choose if we were starting from scratch today. These two are not really mutually exclusive, but in terms of really avoiding FOUC only one is needed.

  1. It's probably worth first going with a turbolinks / pjax style approach.
    That is, each (local) link is intercepted and we make an AJAX request for the HTML, and then we just call an entry point of the loader, bypassing all the necessary scripts. (There's essentially 3 "types" of HTML pages: course, curriculum, and topic pages. Whether a refactored loader would be better off knowing this is TBD.)

Essentially, turn loader.js and the site into a basic SPA.

  1. At this point, I think a build system is a logical alternative approach, and very preferable to a dynamic web app.
    Every build system has their frustrations, but I think a tool like Jekyll or Hugo, etc that lets us define a page template and have includes would be welcome. Personally, I'd also say that moving in the direction of more markdown would be good, too, though there's some challenges with that. No-local-checkout compatibility with GitHub pages would be necessary. Whether that's via actions/CI or just a plain Jekyll install is fine. I don't know, this is more of a conversation...but I guess you see why things haven't changed much.

  2. I think the partial approach of including just stylesheets could work. Part of me says that this will create some friction when things need to change -- but as much as I want a major update there, it seems a little unlikely.
    That said, the biggest change I would like to consider making to the CSS is to set some max-width on the content. We fill WAY wider than almost any site does for content, and I don't think it's good. So, unless we're clever we'd have a different flash of stuff shrinking.

I don't mind the low opacity idea, but I think it would need to communicate "loading..." somehow to not feel broken. But certainly a possibility.

That said, Chris: I do want to accept your contributions. It's just hard to have two PRs that are massive to content with. I realize you can't possibly have all the context for the history of how decisions were made, but it's pretty clear from the code that a single entry point was the goal. GitHub doesn't help with hundreds of files changed, so some guide to reviewing would be helpful. Plus, assuming you wrote a script to update all the content, it would be really helpful to include that. (Since it's a good check over missing content, and we'll probably need to use it in the future if we go down this approach.)

@cycomachead
Copy link
Member

Hi @chrishtr

It's been a couple of months -- do you have any interest in working on this? Does a turbolinks / pjax style approach seem reasonable?

I guess, in any system updates, reducing the async nature of loader would be good.

Perhaps what'd make sense:

  • include links to stylesheets in all pages (with any scripts written included in the repo)
  • options B or C are good.

There are definitely some dependencies in the JS files, so it might be something to be aware of, but I think it's all solvable.
And FWIW, while we're talking about JS -- none of it has been substantially touched in ~5 years, so there's a fair bit of modernization that could happen too. So, if you are interested in (re)writing any JS -- feel free to have at newer APIs. ;)

@chrishtr
Copy link
Author

chrishtr commented Jun 29, 2021 •
edited
Loading

It's been a couple of months -- do you have any interest in working on this?

Hi, yes I am still interested. Since you were busy with the end of semester I held off for a bit, and then my own work overwhelmed me for a while.

Does a turbolinks / pjax style approach seem reasonable?

I could potentially try that. But it means an extra round trip for each page load right?

Perhaps what'd make sense:

  • include links to stylesheets in all pages (with any scripts written included in the repo)
  • options B or C are good.

B and C will help, but there will still be some amount of ugly flashing...

Re Jekyll, etc: I wonder if a simple build system would be good enough. I've been working with a (very simple) one on another project and it turned out to be not that hard. Let me see if I can make it work.

There are definitely some dependencies in the JS files, so it might be something to be aware of, but I think it's all solvable.
And FWIW, while we're talking about JS -- none of it has been substantially touched in ~5 years, so there's a fair bit of modernization that could happen too. So, if you are interested in (re)writing any JS -- feel free to have at newer APIs. ;)

SG, some fun followup opportunities...!

@cycomachead
Copy link
Member

No worries! :) Also happy to google meet / zoom if it's helpful, and/or even an office. (It's kind of weird to say that!)

I could potentially try that. But it means an extra round trip for each page load right?
There should still only be one round trip. The basic flow is preventDefault on a click, an AJAX class for the .html page, then replace body contents, then re-render the page. I mean, naturally the downside is that someone's gotta reimplement a fair bit of what the browser does for you - but at least there's libraries. IIRC, YouTube uses a similar solution. (and I'm not sure if a body.content = X would trigger any different optimizations than a full page load, though I'm sure you have thoughts about how that works!)

It's really only a 2nd page load solution, so we'd still want to do some tidying of the rest of the content loading, but in theory it should help.

B and C will help, but there will still be some amount of ugly flashing...

I guess I'm in the minority here, but I don't think I'd mind a single flash compared to something like a full page spinner / delay all content until ready type approach (e.g. what many SPAs do...). Partially because if all hell breaks loose, at least the html that's presented is still readable.

I am definitely constantly warming up to the idea of including some CSS in the initial page load. I do think it should be a fairly minimal set though (especially since libs like Katex and pygments are decently sized and not used everywhere).

Re Jekyll, etc: I wonder if a simple build system would be good enough. I've been working with a (very simple) one on another project and it turned out to be not that hard. Let me see if I can make it work.

Neat! I am a little weary of bespoke systems since you can end of with a maintenance burden...which I guess is lately what happened with a lot of the code here...

@chrishtr chrishtr mentioned this pull request Aug 15, 2021
Open
@chrishtr
Copy link
Author

Also see #822 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chrishtr @cycomachead @brianharvey