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

Include default server route rules by default #76

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

svara
Copy link
Contributor

@svara svara commented Jan 24, 2025

This PR builds on top of #74 and addresses #71.

Context

Turbo Native allows server-driven routing in Rails applications. Previously, developers had to explicitly define these routing rules in the path config. This PR adds default routing rules for server-driven navigation directly into the framework.

Changes

  • recede, resume and refresh historical location rules (historical location rules) have been added as PathRules.
  • historical location rules are now loaded by default whether or not the source of the path config is provided.
  • the NavigationHierarchyController has been adapted to handle the historical location rules.

Historical Location Behaviour

resume_historical_location

  1. Dismiss the modal view controller.
  2. Arrive back at the view controller on the "default" stack.

refresh_historical_location

  1. Dismiss the modal view controller.
  2. Arrive back at the view controller on the "default" stack.
  3. Refresh the view controller no the "default" stack by revisiting the location.

recede_historical_location

  1. Dismiss the modal view controller.
  2. Arrive back at the view controller on the "default" stack.
  3. Pop the view controller on the "default" stack (unless it's already at the beginning of the backstack).
  4. This will trigger a refresh on the appeared view controller if the snapshot cache has been cleared by a form submission.

@jpsphaxer
Copy link
Contributor

I like this approach @svara - thanks for sharing this!

@svara svara requested review from joemasilotti and jayohms January 29, 2025 14:12
@svara svara marked this pull request as ready for review January 29, 2025 14:12
additionalProperties: PathRule.recedeHistoricalLocation.properties
)
navigator.route(recedeHistoricalLocationProposal)
try await delay()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The delays are far from ideal, but they are necessary to ensure the view lifecycle methods execute properly and the tests pass. I’ll explore architectural improvements in the future.

Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised these are needed even with the TestableNavigationController presenting everything without animation. Does pushing, popping, or dismissing require the delay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the delay the recede historical location test fails. Even though the presentations are not animated, I doubt the view lifecycle methods are all called synchronously. I'll investigate.

Copy link
Contributor

@jayohms jayohms left a comment

Choose a reason for hiding this comment

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

Great work on this @svara. Just a couple questions to start.

Comment on lines 13 to 15
"presentation": "pop",
"visitable": false,
"historical_location": true
Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible that someone includes these historical location paths in their files, attempting to override the default behavior. Are there other properties that we should explicitly provide here? I'm thinking we should at least provide "context": "default" for each of these rules to make it clear these don't operate in the modal context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! They could, but since the default rules are added last, they will always override those in the path config. I'll add "context": "default" and also a test.

patterns: ["/recede_historical_location"],
properties: [
"presentation": "pop",
"visitable": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is visitable already being used in the library? If so, how/where is it being used? Android doesn't have a corresponding property, so I'm curious if there's anything to align here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! It isn't. We use it internally in the email app. I'll remove it.

Comment on lines +219 to +224
guard visit.isHistoricalLocation,
navigationController.presentedViewController != nil else {
return
}

navigationController.dismiss(animated: visit.animated)
Copy link
Member

Choose a reason for hiding this comment

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

The guard with the != nil feels like a lot of inverse logic. What if we used an if statement here instead?

if visit.isHistoricalLocation, navigationController.presentedViewController != nil {
    navigationController.dismiss(animated: visit.animated)
}

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 personally don't have issues with this and use guard extensively for early returns. If you feel strongly about this we can switch to if.

Copy link
Member

Choose a reason for hiding this comment

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

I don't feel strongly, no. It just took me a few reads to understand when dismiss(animated:) would be called.

@svara svara requested a review from jayohms January 29, 2025 17:21
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.

4 participants