The idea was to remove the complexity and weight of vue-router and replace it with a lighter tailored solution that makes the code simpler.
- Maniphest Tasks
- T170459: Cleanup Routing code
- rWIKISTATS2552c7788b7c: Replace vue-router by a (hopefully) lighter solution
- Patch without arc
- git checkout -b D742 && curl -L https://phabricator.wikimedia.org/D742?download=true | git apply
I added a set of unit tests that pass with: npm test.
Initially I wrote it this way, without a specific component (like router-view), because I was trying to make it as simple as possible. But after, I decided to implement the router-link component to make linking easier and shorter. So, maybe it's worth implementing the router-view component as well... dunno.
When a link is pointing to the same page that is being rendered, usually it's called active link.
I left them blank because we still need to define routes for them.
Vue-router didn't need to import router-link all the time. I don't know how they did that.
This method is a bit convoluted, because it tries to magically decide if a link should be marked as current, taking into account that /project/area redirects to /project/area/metric and as such should be marked as current when showing the latter. Whereas /project should not be marked as current when showing /project/area/metric.
I'm not sure of what's our criteria to choose whether to use lodash or do it by hand.
Very cool job and awesome handling of the router's complexity. Just a few comments but no real disagreements :)
Yeah I don't really see much advantage other than that. Also something important to keep in mind is that, unless I'm horribly wrong, the only way of passing properties over to a component using something like RouterView is through URL parameters, so if down the road we want to pass anything to a component, using a RouterView will make it more convoluted.
This makes sense 👌🏼
We probably want to keep the ones that point to stuff in the Wikistats document. As Nuria pointed out it's better to have something external while we don't have a local view.
This is some great effort in making a complex function very clear Marcel :DD
My only concern here is if the complexity of this method will increase when we start adding optional state parameters like range, breakdowns, etc, or if, since we consider those optional, they don't have consideration here.
Nono, we just have this to avoid importing the whole library, you can add as many as you want as long as we're using them :)
Very cool recursion here :)
Only thing is I'd move this to its own handler function and make the constructor a bit lighter
No big deal, but using a different value than "bar" for the path will make the test clearer (to prove the difference between a wildcard and a fixed name)
We might want to return null when a route hasn't been found, like with regexes.
If we add range and breakdown to the routes, it will look like this:
And probably, /project/area will redirect to /project/area/metric/range/breakdown.
The way the code is right now, will mark as current everything that has a redirect that is compatible with the current url. So, I think it would work. I tried to abstract that, so that it would work in the generic case.
I still don't like the way it is expressed very much though... Because all conditions are expressed as negative, for the sake of optimization, but it's really difficult to understand. I have to read it 5 times everytime I want to reason about it. Will think about this and see if I can fix it as part of the rebase :]
Makes total sense, will do.
Yea, I started to code it like you suggest, like using foo, bar, baz and qux...
So... please respond to this comment:
Makes sense, will do.
Using concrete words will make it less confusing, I think, like:
let route = '/allmydonuts/:typeofdonut'; let path = '/allmydonuts/glazedwithlotsofsprinkles'; let result = router.matchPath(route, path); // should not be undefined
I usually oppose to using placeholder words like foo and bar because they don't help in making tests readable :)
Build has FAILED
Link to build: https://integration.wikimedia.org/ci/job/phabricator-jessie-diffs/694/
See console output for more information: https://integration.wikimedia.org/ci/job/phabricator-jessie-diffs/694/console
@mforns the jenkins build is failing: https://integration.wikimedia.org/ci/job/phabricator-jessie-diffs/694/console
Other than that it looks good to me! If you can fix the build problem I'll merge and deploy.