posva / unplugin-vue-router

Next Generation file based typed routing for Vue Router
https://uvr.esm.is
MIT License
1.62k stars 79 forks source link

feat(extendRoutes): allow relative paths in EditableTreeNode #431

Closed robertmoura closed 3 days ago

robertmoura commented 3 months ago

BREAKING CHANGE: allow relative path overrides

Closes https://github.com/posva/unplugin-vue-router/issues/341

robertmoura commented 3 months ago

I'm not entirely sure how the trees are structured or created. I had to update the code to check that the parent was the root. Can you think of any cases where this would not work?

posva commented 3 months ago

Do you have a bug reproduction? This doesn't seem to be fixing something 😅 Changing paths with non absolute routes is not supported yet #341

robertmoura commented 3 months ago

Quite right! It's actually more of a feature 😊 https://github.com/posva/unplugin-vue-router/issues/341 is exactly the problem I was trying to solve 😊

robertmoura commented 3 months ago

I think I'm seeing the problem now. Here's a snippet out of TreeNode. The path is assumed to be relative (a path segment) or absolute if there is a path override. Interesting behaviour... There has to be a bit more thought put into this. The way I'd expect this to work would mean that the current behaviour would be broken. Just going to close this for now 😊 I might come up with something later. We'll see 😄

  get path() {
    return (
      this.value.overrides.path ??
      (this.parent?.isRoot() ? '/' : '') + this.value.pathSegment
    )
  }
robertmoura commented 3 months ago

If we were to make a slight change to the behaviour of the TreeNode's fullPath getter I think relative paths could be supported. It would be a breaking change though. Can you see any reasons why this would be a bad idea?

I just need to think a little about what this would mean for the route block and definePage.

posva commented 3 months ago

The fullPath shouldn’t change its behavior, or at least it shouldn’t be needed

robertmoura commented 3 months ago

I'll have a look at where it is being used and see if I can remove it 😊

robertmoura commented 3 months ago

Sorry, I was overcomplicating this a lot. I didn't realise that you could have absolute paths as child routes. I just assumed you had to put an extra / in the URL or something. Does what I'm doing now make more sense?

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 73.61%. Comparing base (75160e0) to head (16807d7).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #431 +/- ## ========================================== + Coverage 73.29% 73.61% +0.31% ========================================== Files 34 34 Lines 5767 5775 +8 Branches 581 589 +8 ========================================== + Hits 4227 4251 +24 + Misses 1530 1514 -16 Partials 10 10 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

posva commented 3 months ago

Looking good. I will probably give this a review and merge next Monday during the stream

robertmoura commented 3 days ago

Closing in favour of https://github.com/posva/unplugin-vue-router/pull/519