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

Add MutationObserver to Turbo Drive Preloader #911

Closed

Conversation

julianrubisch
Copy link
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
Contributor Author

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

@julianrubisch
Copy link
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
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.

@seanpdoyle
Copy link
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
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
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
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.

None yet

3 participants