-
Notifications
You must be signed in to change notification settings - Fork 430
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
Extract DriveDelegate
interface
#657
base: main
Are you sure you want to change the base?
Conversation
8a4996c
to
9d3cdba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really appreciate the work being done here, this is a really good extraction and refactoring!
I just have one question: what was your thought around having frameMissingFromResponse()
as part of the DriveDelegate
interface?
Why would other potential classes (that have a DriveDelegate
) care about dispatching a warning about missing frames? Isn't that the sole responsibility of the FrameController
?
e95ad6b
to
680b662
Compare
@marcoroth these changes were proposed in the early stages of missing frame handling. I've rebased and moved all frame-related checks to the |
680b662
to
78ef98a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that would explain it!
This looks good from my side! 🥳
e7e3bcd
to
2842524
Compare
42a9f8e
to
6cfc902
Compare
6cfc902
to
b3efade
Compare
@manuelpuyol @kevinmcconnell I'd really appreciate a review of this changeset, if you're able to provide some. |
b3efade
to
009384c
Compare
5ed2662
to
adf43cc
Compare
adf43cc
to
94a7b17
Compare
94a7b17
to
72d170b
Compare
72d170b
to
f67774f
Compare
The `FrameController` depends on the application's singleton `Session` instance in several ways: * it drives the `History` * it determines whether or not an `Element` is "enabled" to navigate a page or frame * it preloads links * it dispatches `turbo:frame-render` and `turbo:frame-load` events * it can initiate a `Visit` This commit codifies those dependent behaviors by creating the `DriveDelegate` interface and declaring the `Session` class as an extension of that interface. With the codification of the `DriveDelegate` responsibilities established, managing `turbo:frame-render` and `turbo:frame-load` events feels out of scope. To resolve that, this commit migrates the `Session.frameRendered` and `Session.frameLoaded` methods to the `FrameController`, and omits them from the `DriveDelegate` interface. The `FrameController` still directly imports the singleton instance of the `Session` from the `src/core/index` module, but assigns it to a `delegate: DriveDelegate` property for use throughout the class. The next step would be to find a way to forward the `session` instance into the [FrameElement.delegateConstructor][] assignment. [FrameElement.delegateConstructor]: https://github.com/hotwired/turbo/blob/3a18111412cb4190b818b96c9c6aad7264cc4441/src/elements/index.ts#L6
f67774f
to
317a5cc
Compare
The
FrameController
depends on the application's singletonSession
instance in several ways:
History
Element
is "enabled" to navigate apage or frame
turbo:frame-render
andturbo:frame-load
eventsVisit
This commit codifies those dependent behaviors by creating the
DriveDelegate
interface and declaring theSession
class as anextension of that interface.
With the codification of the
DriveDelegate
responsibilitiesestablished, managing
turbo:frame-render
andturbo:frame-load
eventsfeels out of scope. To resolve that, this commit migrates the
Session.frameRendered
andSession.frameLoaded
methods to theFrameController
, and omits them from theDriveDelegate
interface.The
FrameController
still directly imports the singleton instance ofthe
Session
from thesrc/core/index
module, but assigns it to adelegate: DriveDelegate
property for use throughout the class.The next step would be to find a way to forward the
session
instanceinto the FrameElement.delegateConstructor assignment.