-
Notifications
You must be signed in to change notification settings - Fork 66
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
Update sub-route when route could be matched #77
Conversation
f71ab79
to
b57253b
Compare
Travis presumably failed due to #64. They are running fine locally. |
Ping, issues are still created regarding this unintuitive behaviour. |
👍 |
👍 |
This is a subtle change. Taking a close look now. |
This is a really nice fix, it tends to more often behave as a reasonable person would expect, and I wasn't able to make it do anything wrong. However.. I'm a bit worried that it's relying on synchronous changes to tail.path not being picked up. I'd like to test it out once I've landed my fix for #44. I'll aim to have a PR out for that today. |
Today I was able to test it on more "production like" cases and it does not work starting from 3rd level: for /#/
for /#/foo
for /#/foo/bar
😢 |
@mgibas Could you create a jsbin to show what you are meaning? I do not see how the nested route could result in |
Here you have one: https://jsbin.com/guguzubixo/1/edit?html,output |
@mgibas You used the version of |
Weird - I was sure I made a copy of this one - my bad then, sorry for waste of time. |
@mgibas No worries, I added a test to make sure no regression happen. Thank you a lot for testing in production, gives us confidence everything else works! |
I'm not sure if it is related and fixes my problem, but I was experiencing problems with route not reflecting in tail.prefix. Single carbon-route element, tail.path is empty, route.path equals "/page". When altering data.page on the same carbon-route, the tail.prefix is null ! |
I ended up overriding the default values of tail.prefix which are equal to null, in case no tail is present.
|
I think I encountered a similar issue as with @myuseringithub <carbon-route route="{{route}}" pattern="/:owner" data="{{owner}}" tail="{{ownerTail}}"></carbon-route>
<carbon-route route="{{ownerTail}}" pattern="/:name" data="{{name}}" active="{{hasProject}}"></carbon-route> If To fix this issue, whenever data is updated, create a new |
cbdc667
to
be8dd86
Compare
Fixed another issue in this area. Instead of mindlessly prepending |
I found an issue with PR version - tail does not seem to be nulled when parent route is not active. |
@mgibas This was actually an issue on master also (this PR did not change the functionality). I have updated this PR to fix that behavior too, as we encountered too today in our application. |
I can only confirm it works now :) gj 👍 |
//this.tail = { path: null, prefix: null, queryParams: null }; | ||
//this.data = {}; | ||
this.tail = { path: null, prefix: null, __queryParams: null }; | ||
this.data = {}; |
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.
Are these changes necessary for this PR to work?
The reason why we've been leaning towards not clearing out tail
and data
when a route no longer matches is that we want to do as little work as possible on views that aren't visible.
Consider the scenario where we've got three pages: /user
/user/:username/profile
and /watch/:videoId
with <user-page>
and <watch-page>
as top level siblings, and <user-page>
using tail
to delegate to a <user-profile-page>
.
When navigating from /user/alice/profile
to /watch/panda-sneeze
we have to do a certain amount of work to set up the panda-sneeze video. From a performance standpoint, it would be nice if we could do the minimum of changes inside <user-profile-page>
at the same time, as it's invisible to the user at that point.
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.
But active
should be set to false in this case right ?
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.
Tangent, but it would probably be worth documenting the rationale for not setting tail
and data
here in a comment.
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 have removed these changes for now, but would like to propose them in a different pull request later.
The performance is obtained if carbon-route
are mutually exclusive. However, our application breaks as this assumption is incorrect there. We have multiple steps in one of our pages which guide the user through a process. This process starts with selecting a project and then a pull request.
Given that the user has not selected a project, a message "Select a repository on the left to start" is shown. Once the user selects a project, we show a list of pull requests with the same process. Then when the user selects a pull request, we switch to a different view.
Now the issue becomes that if you stop the process and go back to the beginning (start over to project selection) all nested carbon-route
are not notified of this change. They still think they are active as they never received the updated tail
data. Therefore our application breaks as it is relying on corrupt and conflicting data.
Nesting of carbon-route
is severely limited due to this behavior. While I do understand the performance improvement, I would kindly ask you to reconsider it.
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.
This does affect me too, I have a route /:page
then a sub-route of /foo/:id
.
If you navigate to /foo/1234
, then to /foo
, the id
property will still be set to 1234
.
Workaround for this is probably to check both active
and data.id
.
Cool, sorry for the long delay, #44 turned out to be trickier than it looked at first. Mind rebasing this against master? |
Add test for trailing slash Fix issue with not propagating tail after updating data Fix issue with nested tail carbon-route becoming active
104d2de
to
fea4d69
Compare
This was a real headache to get working again, but I managed to make my added tests pass with minimal changes to the existing tests. Mostly paths that were previously |
Anything preventing this to be merged - other than time passing by too quickly, I mean ? |
Note to self: Maybe a solution with propagating the parent active value to its children. This is performant, fixes this issue and also fixes the issue with incorrect data being propagated. Will try to implement this after my project has been finished. |
Copying my comment on this PR from slack: I'm really torn about this PR. On the plus side I think the behavior is more intuitive. On the minus side, it strikes me as less consistent and a bit harder to fully understand. i.e. the current slash matching logic is simple, easy to understand, but gets in your way more. This PR makes it a bit subtler but also it's more likely to do the thing that you expect. Getting feedback from folks who've try this out will help break the tie. |
So, is this change essentially making it so an empty tail is Anyhow, I've tried this on a (near production-ready) app and it seems to work fine FYI. So |
I really like this PR, can we please merge it? |
@rictic The number of issues this PR fixes and the fact that "it is more likely to do the thing that you expect" should help making a decision re merging this PR. |
I have submitted a different (and imo better) solution in #125 |
any update on this one? |
This PR is very old and I am no longer pursuing this change. Therefore I am closing this PR. |
This change makes sure that when a route could be matching and a value is updated, it correctly reflects in the url.
E.g. User is at
domain.com/foo
and the following twocarbon-route
exist:When (through data-binding)
myPage
is updated topage
, the url now showsdomain.com/foo/page
.This change should be more intuitive in handling sub-pages and possible redirects.
Fixes #75
Fixes #37