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

Make Transition#urlMethod public #180

Closed
wants to merge 1 commit into from

Conversation

bendemboski
Copy link

Document this property and make it public so route hooks can detect the type (URL method) of transition that is occurring.
#169 documented the Transition class and specified several methods and properties as public or private. This is a follow-up in which I am proposing that we document the Transition#urlMethod property and make it public. The Transition#method method that updates it is already public, so it seems to make sense to make it public as well, so there is a public way of reading it as well as writing it.

Background

This issue describes a problem where a route that performs a redirect needs to know whether the transition's target URL is already in the history or not so it knows whether to call replaceWith() or transitionTo() in order to keep the navigation history correct.

The workaround described in the comments involves using the Transition#sequence property, which is currently private, only used for logging, and set based on an incrementing static class property that is not reset when the app resets (e.g. in unit tests). In short, a hack.

Making Transition#urlMethod public would provide a much cleaner workaround for the bug. Even ignoring that issue, it seems plausible that certain routes might want to make different decisions based on whether the transition they are handling is doing a replace, update, or not modifying the history.

Document this property and make it public so route hooks can detect the type (URL method) of transition that is occurring.
@alexspeller
Copy link
Contributor

Fix for the root cause of wanting this at #197

@bendemboski
Copy link
Author

Awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants