Skip to content

Add MutationObserver to Turbo Drive Preloader#911

Closed
julianrubisch wants to merge 2 commits intohotwired:mainfrom
julianrubisch:preloader-mutation-observer
Closed

Add MutationObserver to Turbo Drive Preloader#911
julianrubisch wants to merge 2 commits intohotwired:mainfrom
julianrubisch:preloader-mutation-observer

Conversation

@julianrubisch
Copy link
Copy Markdown
Contributor

Alternative to #910

@domchristie and @marcoroth both pointed out in private conversations that exposing the preloader as public API could lead to a lot of maintenance overhead

so I took a spike at rather enhancing it with a MutationObserver that preloads links of anchor elements (and only those) when data-turbo-preload is added to them dynamically.

Let me know what you think!

@julianrubisch
Copy link
Copy Markdown
Contributor Author

heads up: I've rewritten this in pure JS because I really think it'd be beneficial 🙌

@julianrubisch julianrubisch force-pushed the preloader-mutation-observer branch from 8ae96cf to 06d66fc Compare October 5, 2023 07:09
@julianrubisch
Copy link
Copy Markdown
Contributor Author

Added a test ✌

Comment on lines +8 to +14
this.preloadLinkObserver = new MutationObserver((mutationList, observer) => {
mutationList.forEach((mutation) => {
if (mutation.attributeName !== "data-turbo-preload" || mutation.target.tagName !== "A") return

this.preloadURL(mutation.target)
})
})
Copy link
Copy Markdown
Contributor

@seanpdoyle seanpdoyle Oct 12, 2023

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
Copy Markdown
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.

@seanpdoyle
Copy link
Copy Markdown
Contributor

I think this is a great patch! I'm curious if you're interested in exploring #911 (comment) while we wait for a maintainer to grant the branch permission to execute CI.

@pfeiffer
Copy link
Copy Markdown
Contributor

This is great! Having a mutation observer in place to handle preloading would allow dynamically controlling preloading of links on hover, eg. similar to "hoverintent". We use a similar pattern today but misuse the private Turbo.session.preloader.

@seanpdoyle
Copy link
Copy Markdown
Contributor

While I think the MutationObserver changes in this PR are a big improvement, I've opened #1033 to resolve some other outstanding issues that I've uncovered while poking around during code review for this PR.

@julianrubisch
Copy link
Copy Markdown
Contributor Author

And another one gone...

@julianrubisch julianrubisch deleted the preloader-mutation-observer branch February 5, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants