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

Add MutationObserver to Turbo Drive Preloader #911

Closed
wants to merge 2 commits into from
Closed

Add MutationObserver to Turbo Drive Preloader #911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
06d66fc
chore: Rewrite preloadLinkObserver in pure JS
julianrubisch Oct 5, 2023
ec65690
test: Add dynamic preloader test
julianrubisch Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/core/drive/preloader.js
View file
Open in desktop
Expand Up @@ -5,6 +5,13 @@ export class Preloader {

constructor(delegate) {
this.delegate = delegate
this.preloadLinkObserver = new MutationObserver((mutationList, observer) => {
mutationList.forEach((mutation) => {
if (mutation.attributeName !== "data-turbo-preload" || mutation.target.tagName !== "A") return

this.preloadURL(mutation.target)
})
})
Comment on lines +8 to +14
Copy link
Contributor

Choose a reason for hiding this comment

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

While I appreciate that this PR's code matches the style of the surrounding code, I wonder if the introduction of the MutationObserver poses an opportunity to re-structure how the Preloader monitors the page.

There's some precedent for code tied to the document's structure, nodes, and attributes to live in the src/observers directory as a class with a .start() and .stop() method and a context-specific delegate interface (when the project was built with TypeScript, enforcing that delegate interface was fairly automatic).

In this case, calling .start() could attach the MutationObserver (maybe even with attributeFilter: "data-turbo-preload"), and calling .stop() could detach it.

I'm imagining a PreloadObserver resembling something like:

class PreloadObserver {
  started = false
  attributeName = "data-turbo-preload"

  constructor(delegate, element) {
    this.delegate = delegate
    this.element = element
    this.mutationObserver = new MutationObserver(this.#notifyDelegateOfMutations)
  }

  start() {
    if (!this.started) {
      this.mutationObserver.observe(this.element, { attributeFilter: [this.attributeName], subtree: true })

      for (const element of this.element.querySelectorAll(`[${this.attributeName}]`)) {
        this.#notifyDelegate(element)
      }
  
      this.started = true
    }
  }

  stop() {
    if (this.started) {
      this.mutationObserver.disconnect()
      this.started = false
    }
  }

  #notifyDelegateOfMutations = (mutationList) => {
    for (const { target } of mutationList) {
       this.#notifyDelegate(target)
    }
  }

  #notifyDelegate = (element) => {
    if (element instanceof HTMLAnchorElement) {
      // this is a bad method name, but communicates the point
      this.delegate.canPreloadAnchor(element)
    }
  }
}

Then, the Session instance could build an observer (and manage its .start() and .stop() calls with the rest of its observers:

export class Session {
  // ...
  preloadObserver = new PreloadObserver(this, document.body)

  start() {
    // ...
    this.preloadObserver.start()
    // ...
  }

  stop() {
    // ...
    this.preloadObserver.stop()
    // ...
  }

  // Preload observer delegate

  canPreloadAnchor(anchor) {
    this.preloader.preloadURL(anchor)
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I very much like this suggestion, but please forgive me if I don't lift a finger until I hear back from one of the maintainers.

}

get snapshotCache() {
Expand All @@ -15,9 +22,11 @@ export class Preloader {
if (document.readyState === "loading") {
return document.addEventListener("DOMContentLoaded", () => {
this.preloadOnLoadLinksForView(document.body)
this.observeLinksForView(document.body)
})
} else {
this.preloadOnLoadLinksForView(document.body)
this.observeLinksForView(document.body)
}
}

Expand All @@ -27,6 +36,13 @@ export class Preloader {
}
}

observeLinksForView(element) {
this.preloadLinkObserver.observe(element, {
attributes: true,
subtree: true
})
}

async preloadURL(link) {
const location = new URL(link.href)

Expand Down
16 changes: 16 additions & 0 deletions src/tests/fixtures/dynamic_preloading.html
View file
Open in desktop
@@ -0,0 +1,16 @@
<!DOCTYPE html>
<html>

<head>
<meta charset="utf-8">
<title>Preloading Page</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
</head>

<body>
<a href="/src/tests/fixtures/preloaded.html" id="preload_anchor">
Visit preloaded page
</a>
</body>

</html>
19 changes: 19 additions & 0 deletions src/tests/functional/preloader_tests.js
View file
Open in desktop
Expand Up @@ -51,3 +51,22 @@ test("test navigates to preloaded snapshot from frame", async ({ page }) => {
})
)
})

test("test preloads snapshot after adding data-turbo-preload attribute at runtime", async ({ page }) => {
await page.goto("/src/tests/fixtures/dynamic_preloading.html")
await nextBeat()

await page.evaluate(async () => {
document.querySelector("#preload_anchor").setAttribute("data-turbo-preload", "true")
})
await nextBeat()

assert.ok(
await page.evaluate(async () => {
const preloadedUrl = new URL("http://localhost:9000/src/tests/fixtures/preloaded.html")
const cache = window.Turbo.session.preloader.snapshotCache

return await cache.has(preloadedUrl)
})
)
})