Page load improvements #2007

liamcmitchell · 2025-01-07T16:45:38Z

support importing plain .html templates, bypassing handlebars completely
sidebar-list:
- remove redundant ids & classes, prefer using aria attrs
- small adjustment to expand button position
- initialize on sidebar open - no need to do anything until visible
- init once only, no need to re-render on nav - NOTE: expand/collapse states will now persist across SWUP nav
- convert from handlebars to vanilla JS
- move tab and tabset render from elixir to JS - keep it all together, less HTML
- remove nodes_map from many functions
- much inlined
- refactored callbacks
copy-button:
- move button to template
- create template lazily
- inline addCopyButtons
modal:
- convert to html template
- move state to plain vars intead of nested properties, no need for object
- remove init, render lazily
- inlined more things
quick-switch:
- convert body to html template
- build result el inline
sidebar-drawer:
- register update callback
- call sidebar-list init on open
tabsets: get heading text without requiring layout
toast: init lazily

Before

../formatters/html/dist/html-OD6LFIR3.js                             144.0kb
../formatters/html/dist/html-elixir-FAAZMXJW.css                      65.4kb

After

../formatters/html/dist/html-HTRMLDRB.js                             130.2kb
../formatters/html/dist/html-elixir-EGPMA5T5.css                      64.4kb

-14kb JS, -1kb CSS, no more forced layout in init, sidebar-list renders about 4x faster

github-actions · 2025-01-07T16:46:50Z

📦 Docs artifacts are ready: https://github.com/elixir-lang/ex_doc/actions/runs/12655533885/artifacts/2396847244

josevalim · 2025-01-07T19:47:15Z

It looks so smooth now, thank you! 💚 💙 💜 💛 ❤️

josevalim · 2025-01-07T19:47:26Z

💚 💙 💜 💛 ❤️

josevalim · 2025-01-07T19:51:33Z

@liamcmitchell I found a bug when running it under swup.js. To reproduce:

$ mix build
$ python3 -m http.server 8080
$ open http://localhost:8080/doc/

If you click on the sidebar to navigate across pages, for example within "Pages", it doesn't close the current open item. Can you please take a look?

I am wondering if we still need swup.js but I would say the answer is yes to avoid flashing pages on slow connections?

jonatanklosko · 2025-01-08T03:49:13Z

assets/js/modal.js

+// State
+
+/** @type {HTMLDivElement | null} */
+let modal = null
+/** @type {HTMLElement | null} */
+let prevFocus = null
+/** @type {HTMLElement | null} */
+let lastFocus = null
+let ignoreFocusChanges = false


FWIW I consider a single state object good for maintainability/readability, because it makes it clear when we modify the global state rather than deal with local variables :)

I assumed it was done for that reason (I've also done something similar in the past) but I had my code-golfing hat on.

I don't think the global/local distinction is very useful here. To me, global is JS global (window/global), everything else is local. I don't see why module state needs to be treated differently to function state. The let keyword should signal that the var is state and it should be no harder to trace back. Maybe rename it to let stateModal or similar if it needs to be special? But I don't think it's special.

Anyway, reasons to avoid the object wrapper:

one less object

property accesses have a runtime cost

i think code is easier to read without state. everywhere

i don't think it can be minified as well

object: let o={modal:null};o.modal

let: let m=null;m

Super minor I know but I didn't think the object wrapper was very useful.

I don't see why module state needs to be treated differently to function state

Function state is short lived and usually it's easy to see all variables the function defines. So the rationale would be that reading a function 100 lines into the file, seeing state.modal = ... immediately gives the idea that it alters a long-lived state (like a state store), while model = ... looks just like a function-local assignment and you need to look around to rule that out.

I wouldn't expect the runtime cost of key access to be relevant in practice, but minification is a fair argument.

That said, it's not a strong opinion and I don't mind the change. Just mentioning for completeness :)

jonatanklosko · 2025-01-08T04:27:05Z

assets/js/handlebars/helpers.js

-Handlebars.registerHelper('groupChanged', function (context, nodeGroup, options) {
-  const group = nodeGroup || ''
-  if (context.group !== group) {
-    // reset the nesting context for the #nestingChanged block helper
-    delete context.nestedContext
-    context.group = group
-    return options.fn(this)
-  }
-})
-
-Handlebars.registerHelper('nestingChanged', function (context, node, options) {
-  // context.nestedContext is also reset each time a new group
-  // is encountered (the value is reset within the #groupChanged
-  // block helper)
-  if (node.nested_context && node.nested_context !== context.nestedContext) {
-    context.nestedContext = node.nested_context
-
-    if (context.lastModuleSeenInGroup !== node.nested_context) {
-      return options.fn(this)
-    }
-  } else {
-    // track the most recently seen module
-    // prevents emitting a duplicate entry for nesting when
-    // the nesting prefix matches an existing module
-    context.lastModuleSeenInGroup = node.title
-  }
-})


This is great! FTR if someone is motivated enough we could drop handlebars altogether. We could build the HTML by hand x.innerHTML = `...` or perhaps use something lightweight like lit-html or similar (which relies on tagged string literals such as html`<div>${foo}</div>`).

We'll see how far I get. I've been replacing most of it with vanilla JS and an el() helper:

ex_doc/assets/js/helpers.js

Lines 203 to 222 in 22103e7

/**

* Create element from tag, attributes and children.

*

* @param {string} tagName

* @param {Record<string, any>} attributes

* @param {(HTMLElement | string)[]} [children]

* @returns {HTMLElement}

*/

export function el (tagName, attributes, children) {

const element = document.createElement(tagName)

for (const key in attributes) {

if (attributes[key] != null) {

element.setAttribute(key, attributes[key])

}

}

if (children) {

element.replaceChildren(...children)

}

return element

}

el() is totally fine also! Unless the code ends up overly nested/convoluted for the slightly bigger handlebars templates (probably manageable though).

Yeah, there is definitely value in templates but the handlebars runtime alone is 50kb.

I'm looking at the remaining logic in handlebars and thinking about how it would look as a mix of plain HTML templates and vanilla JS.
The most complex templates are:

autocomplete-suggestions.handlebars

search-results.handlebars

tooltip-body.handlebars

I think it's doable but prob also worth investigating Svelte, which has a very similar template syntax but much smaller runtime cost.

I think I'll try converting one of them both ways and compare.

Exactly, handlebars runtime dependency is a good reason to get rid of that.

I mentioned lit-html, because it is much smaller than something like Svelte, and for rendering small chunks of html it should be enough.

I was wrong about the handlebars runtime size, I must have remembered the size of the templates + runtime.

I implemented tooltip-body in vanilla, svelte and lit-html to get a better idea of how it looks and see runtime and template sizes.

Handlebars (current)

runtime = 17kb
template = 1.6kb

{{#if isPlain}} <section class="docstring docstring-plain"> {{this.hint.description}} </section> {{else}} <div class="detail-header"> <h1 class="signature"> <span translate="no">{{this.hint.title}}</span> <div class="version-info" translate="no">{{this.hint.version}}</div> </h1> </div> {{#if this.hint.description}} <section class="docstring"> {{{this.hint.description}}} </section> {{/if}} {{/if}}

tooltipBody.innerHTML = tooltipBodyTemplate({ isPlain: hint.kind === HINT_KIND.plain, hint })

VanillaJS

runtime = <1kb
template = 0.4kb
diff

tooltipBody.replaceChildren(...( hint.kind === HINT_KIND.plain ? [el('section', {class: 'docstring docstring-plain'}, [hint.description])] : [ el('div', {class: 'detail-header'}, [ el('h1', {class: 'signature'}, [ el('span', {translate: 'no'}, [hint.title]), el('div', {class: 'version-info', translate: 'no'}, [hint.version]) ]) ]), hint.description && el('section', {class: 'docstring'}, nodesFromHtml(hint.description)) ].filter(Boolean) ))

Svelte 5

runtime = 24kb
template = 0.7kb
diff

<script> const { isPlain, hint } = $props() </script> {#if isPlain} <section class="docstring docstring-plain"> {hint.description} </section> {:else} <div class="detail-header"> <h1 class="signature"> <span translate="no">{hint.title}</span> <div class="version-info" translate="no">{hint.version}</div> </h1> </div> {#if hint.description} <section class="docstring"> {@html hint.description} </section> {/if} {/if}

mount(tooltipBodyTemplate, { target: tooltipBody, props: { isPlain: hint.kind === HINT_KIND.plain, hint } })

Lit-HTML

runtime = 8kb
template = 0.4kb
diff

render( hint.kind === HINT_KIND.plain ? html`<section class="docstring docstring-plain">${hint.description}</section>` : html` <div class="detail-header"> <h1 class="signature"> <span translate="no">${hint.title}</span> <div class="version-info" translate="no">${hint.version}</div> </h1> </div> ${hint.description && html`<section class="docstring">${unsafeHTML(hint.description)}</section>`} `, tooltipBody )

Thanks for suggesting lit-html, don't remember hearing about it before. Cool that GH highlights the template strings too.

I had expected the svelte runtime to be leaner but apparently they added more with v5 to make the templates smaller. Still impressive that it has full reactivity and is not much bigger than handlebars.

Great comparison!!

I actually didn't hear about it either, but I searched around for html`...` literal, since I had a vague idea that I either saw it somewhere or it should exist as an optimised version of x.innerHTML = `...` :D There is also lighterhtml (and probably others), but based on popularity and activity lit-html is definitely a safer bet. The reason I would pick it over Svelte here is that the html literal feels almost exactly like a regular interpolated string, so there is no abstraction overhead on contributors. So for our purpose, I would lean towards that, but if you have more thoughts, happy to hear them!

liamcmitchell · 2025-01-08T08:30:49Z

@josevalim

If you click on the sidebar to navigate across pages, for example within "Pages", it doesn't close the current open item. Can you please take a look?

I mentioned this in the PR desc, maybe I should have bolded more :)

init once only, no need to re-render on nav - NOTE: expand/collapse states will now persist across SWUP nav

I thought it was more a feature than a bug. I thought if a user has opened a menu, then closing it without explicit intent (clicking collapse button) might be disruptive. I can also see that auto-closing menus keeps the menu clean. I'll make a PR to change it.

I am wondering if we still need swup.js but I would say the answer is yes to avoid flashing pages on slow connections?

I was wondering the same thing. With the improvements we've made so far, there is very little flashing without swup. Browsers keep showing the old page until the new is ready to render (all CSS is loaded and some body HTML is parsed). When CSS & JS is cached the visible flash will depend on how fast the client can parse the HTML and execute the init JS.

With swup:

with-swup.mov

Menu could be updated immediately, no need to wait for content load.

Without swup:

without-swup.mov

Note this could be faster if my dev server set cache header with max-age > 0. You see reqs to revalidate assets in network tab.
JS bundle is 30kb smaller.

josevalim · 2025-01-08T10:55:48Z

I mentioned this in the PR desc, maybe I should have bolded more :)

It was my bad then. :) Perhaps we can have "breaking changes" listed separately?

I thought it was more a feature than a bug. I thought if a user has opened a menu, then closing it without explicit intent (clicking collapse button) might be disruptive. I can also see that auto-closing menus keeps the menu clean. I'll make a PR to change it.

I thought about this but I'd prefer for now to keep a consistent experience across them.

Menu could be updated immediately, no need to wait for content load.

Originally, I was think we should update the menu immediately and clear the content area, but when I looked at other places, like Docusaurus, it behaves like today. What are your general thoughts?

Other than that, given we still see the sidebar flashing without swup.js, we should probably keep it in?

liamcmitchell · 2025-01-08T11:30:15Z

Originally, I was think we should update the menu immediately and clear the content area, but when I looked at other places, like Docusaurus, it behaves like today. What are your general thoughts?

I think it's good as-is. Doing all changes together keeps the nav consistent with visible content and means less layout shift.

Other than that, given we still see the sidebar flashing without swup.js, we should probably keep it in?

I think the experience is worth the 30kb. I would keep it.

josevalim · 2025-01-09T18:14:13Z

Btw, this pull request breaks the sidebar rendering on iOS. No icons show up. I am trying to understand why it happens, but nothing is logged to the console. :D

josevalim · 2025-01-09T18:25:49Z

Ok, it happens on any small screen. So if you reduce the browser width, you can reproduce it. I found the exact line of code, I will drop a comment.

josevalim · 2025-01-09T18:26:32Z

assets/js/sidebar/sidebar-drawer.js

@@ -45,6 +48,7 @@ export function initialize () {
 export function update () {
  const pref = sessionStorage.getItem(SIDEBAR_STATE_KEY)
  const open = pref !== SIDEBAR_PREF_CLOSED && !isScreenSmall()
+  if (open) initializeList()


This is the problematic line. It doesn't ever initialize the list on small screens. :) I will revert it on main but feel free to push a better fix if this should be maintained somehow.

liamcmitchell added 7 commits January 7, 2025 14:33

Init modal lazily

3af7992

Quick-switch improvements

df4cb08

Init toast lazily

96d1fea

Render sidebar list in vanilla JS

5a481f4

Set tabset headings without layout

1ded546

Move copy button to template and parse lazily

6144d4a

Use el helper in quick-switch

ed5d015

josevalim merged commit c7bae84 into elixir-lang:main Jan 7, 2025
4 checks passed

jonatanklosko reviewed Jan 8, 2025

View reviewed changes

liamcmitchell deleted the assets branch January 8, 2025 07:27

liamcmitchell mentioned this pull request Jan 8, 2025

Close open menus on nav #2010

Merged

josevalim reviewed Jan 9, 2025

View reviewed changes

liamcmitchell mentioned this pull request Jan 9, 2025

Fix sidebar open/close on mobile #2016

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Page load improvements #2007

Page load improvements #2007

liamcmitchell commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

josevalim commented Jan 7, 2025

josevalim commented Jan 7, 2025

josevalim commented Jan 7, 2025

jonatanklosko Jan 8, 2025

liamcmitchell Jan 8, 2025 •

edited

Loading

jonatanklosko Jan 8, 2025

jonatanklosko Jan 8, 2025

liamcmitchell Jan 8, 2025

jonatanklosko Jan 8, 2025

liamcmitchell Jan 8, 2025

jonatanklosko Jan 8, 2025 •

edited

Loading

liamcmitchell Jan 8, 2025

jonatanklosko Jan 8, 2025 •

edited

Loading

liamcmitchell commented Jan 8, 2025

josevalim commented Jan 8, 2025

liamcmitchell commented Jan 8, 2025

josevalim commented Jan 9, 2025

josevalim commented Jan 9, 2025

josevalim Jan 9, 2025

	/**
	* Create element from tag, attributes and children.
	*
	* @param {string} tagName
	* @param {Record<string, any>} attributes
	* @param {(HTMLElement \| string)[]} [children]
	* @returns {HTMLElement}
	*/
	export function el (tagName, attributes, children) {
	const element = document.createElement(tagName)
	for (const key in attributes) {
	if (attributes[key] != null) {
	element.setAttribute(key, attributes[key])
	}
	}
	if (children) {
	element.replaceChildren(...children)
	}
	return element
	}

Page load improvements #2007

Page load improvements #2007

Conversation

liamcmitchell commented Jan 7, 2025

Before

After

github-actions bot commented Jan 7, 2025

josevalim commented Jan 7, 2025

josevalim commented Jan 7, 2025

josevalim commented Jan 7, 2025

Choose a reason for hiding this comment

liamcmitchell Jan 8, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatanklosko Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Handlebars (current)

VanillaJS

Svelte 5

Lit-HTML

jonatanklosko Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

liamcmitchell commented Jan 8, 2025

josevalim commented Jan 8, 2025

liamcmitchell commented Jan 8, 2025

josevalim commented Jan 9, 2025

josevalim commented Jan 9, 2025

Choose a reason for hiding this comment

liamcmitchell Jan 8, 2025 •

edited

Loading

jonatanklosko Jan 8, 2025 •

edited

Loading

jonatanklosko Jan 8, 2025 •

edited

Loading