Page MenuHomePhabricator

Replace vue-router by a (hopefully) lighter solution
ClosedPublic

Authored by mforns on Aug 9 2017, 4:51 PM.

Details

Maniphest Tasks
T170459: Cleanup Routing code
Reviewers
fdans
Milimetric
Commits
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
Summary

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.

Test Plan

I added a set of unit tests that pass with: npm test.

Diff Detail

Event Timeline

mforns created this revision.Aug 9 2017, 4:51 PM
mforns added inline comments.Aug 10 2017, 10:57 AM
src/App.vue
9

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.
I guess the only advantage would be that App.vue wouldn't need to import and declare all the components.

54

When a link is pointing to the same page that is being rendered, usually it's called active link.
But at the same time, that link is inactive, because it's not clickable. I think it's very confusing.
That's why I changed it to: router-link-current.

src/components/BottomFooter.vue
4

I left them blank because we still need to define routes for them.

21

Vue-router didn't need to import router-link all the time. I don't know how they did that.

src/components/RouterLink.vue
25

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.

src/lodash-custom-bundle.js
22

I'm not sure of what's our criteria to choose whether to use lodash or do it by hand.
If I added too many lodash methods, please scold.

fdans edited edge metadata.EditedAug 17 2017, 12:37 PM

Currently reviewing this but in the meantime you should merge develop or rebase, as the current base of this branch is quite old :)

Very cool job and awesome handling of the router's complexity. Just a few comments but no real disagreements :)

src/App.vue
9

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.

54

This makes sense 👌🏼

src/components/BottomFooter.vue
4

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.

src/components/RouterLink.vue
25

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.

src/lodash-custom-bundle.js
22

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 :)

src/router/index.js
37

Very cool recursion here :)

154–172

Only thing is I'd move this to its own handler function and make the constructor a bit lighter

test/Router.spec.js
35

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)

48

We might want to return null when a route hasn't been found, like with regexes.

mforns added inline comments.Aug 23 2017, 12:12 PM
src/components/RouterLink.vue
25

If we add range and breakdown to the routes, it will look like this:

/project/area/metric/range/breakdown

And probably, /project/area will redirect to /project/area/metric/range/breakdown.
Also, the links to other metrics in the left side menu will be /project/area/metric and will redirect as well 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 :]

src/router/index.js
154–172

Makes total sense, will do.

test/Router.spec.js
35

Yea, I started to code it like you suggest, like using foo, bar, baz and qux...
But for more complex tests, it was also really confusing, because I was checking that
:foo == bar and :baz == qux. So, in the end I decided to go with :foo == foo which, even if as you say may be confusing because you can mistake a wildcard with a fixed name, was clear that foo was matching with :foo, and bar with :bar, etc.
Dunno! I think both ways have advantages and disadvantages.

So... please respond to this comment:
If you say 'foo' I will change it as you suggest.
If you say 'bar' I will leave it as is.
:}

48

Makes sense, will do.

fdans added inline comments.Aug 28 2017, 4:14 PM
test/Router.spec.js
35

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 :)

mforns updated this revision to Diff 2013.Aug 31 2017, 5:13 PM

Rebase on top of develop branch

fdans added a comment.Sep 4 2017, 12:08 PM

@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.

fdans updated this revision to Diff 2023.Sep 5 2017, 2:57 PM
  • Merge branch 'develop' of ssh://git-ssh.wikimedia.org/source/wikistats into arcpatch-D742_1
fdans updated this revision to Diff 2024.Sep 5 2017, 3:15 PM
  • resolves routerview conflict correctly
fdans accepted this revision.Sep 5 2017, 4:12 PM

Just solved one conflict that was pending and synced with develop

This revision is now accepted and ready to land.Sep 5 2017, 4:12 PM
This revision was automatically updated to reflect the committed changes.

Thanks a lot @fdans !