-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add lifecycle methods to route config #172
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
base: master
Are you sure you want to change the base?
Conversation
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.
@matt-sneed with the additions of the life cycle functions to the route data, are we able to support a router use case where no router mixin is used? It seems like the changes allow for that, but without a concrete example I just wanted to verify.
* @param {string} routeId - The ID of the route being entered. | ||
* @param {!Object} context - A context object, potentially containing shared state or utilities. | ||
* @returns {Promise<boolean|void>} Should return a Promise. | ||
* Resolves to `true` or `void` to allow navigation. |
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.
Does it semantically make sense to allow the routEnter()
callback to have a return value? My mental model is that the transition has already occurred by the time that hook is called so the window to abort would have passed.
I also notice that we don't have symmetry between the exit and enter hooks (exit lacks a "before" flavor). Could these just be collapsed into one enter and one exit (the way the mixin does) and have them return boolean
values to indicate if the transition can occur?
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.
@C-Duxbury i was just recreating the same logic that was currently there, but for the routeData version. i believe the intention is to prevent recursing down the node list so that if a parent returned false in it's lifecycle method it would prevent further tree navigation.
web-component-router/lib/route-tree-node.js
Line 339 in 5252535
const shouldContinue = await routingElem.routeEnter(currentEntryNode, nextEntryNode, routeId, context); |
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 edited my comment to expand on this. Should we just have a single "enter" hook that occurs before and can return a boolean
value to indicate if the transition can occur? I'm wondering if there's value in having distinct before/after hooks for route enter since the mixin doesn't.
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 can't think of a use case that needs both unless there is some case for performing since actions after committing to entering a route. Seems reasonable to have just the single case. @matt-sneed ?
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.
Agreed, this is what I'm picturing:
entry
if routeData.beforeEnter(...) !== false
get or create route component
if routeComponent.routeEnter
routeComponent.routeEnter(...)
exit
if routeData.beforeExit(...) !== false
get exit component
if routeComponent.routeExit
routeComponent.routeExit(...)
So the RouteData
in the config only provides the beforeEnter
and/or beforeExit
hooks. The components can still implement routeEnter
and/or routeExit
but are not required to do so.
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 might use routeEnter to navigate my page down to a anchor on the page based on the route path
@matt-sneed Do we need router-specific hooks to satisfy this use case? My thought is that connectedCallback()
or firstUpdated()
could be used for this purpose. The component doesn't need to have knowledge about routing to do something when it's placed into the DOM.
I think we can probably get away with just having one enter and exit hook each in the config to allow actions/guards that happen before the transition occurs and the element is placed in the DOM.
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.
For example, the router config shouldn't be directly using fetch and updating data, nor should it be manipulating the DOM
@jrobinson01 agree with that statement, i'm saying that the router provides the "hook" to let the app do that job. now part of the equation is what lifecycle hooks are needed to do all the use cases we might need.
Do we need router-specific hooks to satisfy this use case? My thought is that connectedCallback() or firstUpdated() could be used for this purpose. The component doesn't need to have knowledge about routing to do something when it's placed into the DOM.
I think we can probably get away with just having one enter and exit hook each in the config to allow actions/guards that happen before the transition occurs and the element is placed in the DOM.
@C-Duxbury here's a couple pointed examples of where that may not work, and using a routeEnter in a component is still needed:
override async routeEnter(
currentNode: RouteTreeNode,
nextNodeIfExists: RouteTreeNode,
routeId: string,
context: Context,
): Promise<boolean | undefined> {
// store path so it can be passed to sidebar
this.routePath = context.path;
await super.routeEnter(currentNode, nextNodeIfExists, routeId, context);
if (context.hash) {
await this.navToSection(`#${context.hash}`);
}
return true;
}
override async routeEnter(
currentNode: RouteTreeNode,
nextNodeIfExists: RouteTreeNode,
routeId: string,
context: Context,
) {
// store path so it can be passed to sidebar
await super.routeEnter(currentNode, nextNodeIfExists, routeId, context);
this.reversalFlag = context.query.has('reversalFlag');
}
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.
@matt-sneed We'd need a way to get at the route context directly from the Router
instance to support this use case without hooks. You'd basically just move the logic from routerEnter()
into firstUpdated()
:
protected firstUpdated() {
const { hash } = Router.getInstance().getContext();
if (hash) {
await this.navToSection(`#${hash}`);
}
}
This is one of the gaps I mentioned in the comparison document. Getting the context outside of hooks would be a separate enhancement PR, but I think it's orthogonal with the goal here of providing ways to route without inheritance.
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.
yeah, i think if we had access to the current route context in some consumable way at any point in the lifecycle, it does change the requirements
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.
The router currently reuses components that are already in the tree, so for an existing component, the router would need to have changed one of the component's properties in order to trigger the lit lifecycle events. That might be sufficient for most (all?) use cases where a component needs to react to a route change but maybe worth pointing out here.
i think this is a first step towards that. i don't think it implicitly solves that problem yet. |
Technically, you don't have to use the mixin right now. The only requirement is that each routed element implements the |
I'm creating a demo app in the repo which will allow for better testing of improvements and changes. Lets hold off on PRs until we have that as a sandbox. |
@barronhagerman Is that true in practice? To the best of my ability, it looks like the |
The mixin's |
This adds the beforeEnter lifecycle method as a top level lifecycle method in the router like routeEnter and routeExit. It also provides optionality to setup beforeEnter, routeEnter, routeExit methods in the route config and/or the overriden methods like we have today.
Testing:
Example routing configuration with new lifecycle methods: