Skip to content
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

feat(client): use visual routes #1433

Closed
wants to merge 7 commits into from
Closed

feat(client): use visual routes #1433

wants to merge 7 commits into from

Conversation

Mister-Hope
Copy link
Member

No description provided.

@Mister-Hope Mister-Hope linked an issue Nov 20, 2023 that may be closed by this pull request
@Mister-Hope
Copy link
Member Author

Mister-Hope commented Nov 20, 2023

This pr reduce outputsize and improves performance at the same time. by avoiding:

  1. heavy operation with router.resolve

router.resolve performs RegExp.exec() when mapping the whole routes array to find a matched result.

  1. duplicated route records with redirects and maps holding data, components, now a single map holds all pages info and pageKey is no longer used at client.

Breaking changes

  • pageKey is removed at client, so operations like router.push(pageKey) router.resolve(pageKey) no longer work

  • routeMeta is renamed to meta

    We want to decouple meta concept with router

    routeMeta is deprecated but still supported for backwards compatibility, we should remove the support in stable.

  • People should use resolve in @vuepress/client to get page meta

    Getting page meta with router.resolve is deprecated but still supported for backwards compatibility, we should remove the support in stable. (We can support this for sure, but I think that decouple meta with routes could be better)

    Note: Calling router.resolve to get a correct page path is still supported as this is a correct usage. (but since it's heavy, maybe we should warn users to use resolve as first choice)

Other notes

To support clean url (e.g.: navigating to /a/b and resolve /a/b.html to /a/b), we improved generation of htmlRelativePath and htmlPath on Page object

Comment on lines 51 to 58
// if no match at this point, then we should provide 404 page
const pageInfo = pagesMap.get(pagePath) || pagesMap.get('/404.html')!

// TODO: Added for backwards compatibility, remove in stable version
to.meta = pageInfo.meta
;[to.meta._data] = await Promise.all([
resolvers.resolvePageData(to.name as string),
pagesComponents[to.name as string]?.__asyncLoader(),
resolvers.resolvePageData(pageInfo, pagePath),
pageInfo.comp?.__asyncLoader(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notes:

Since we use beforeResolve to align pageData with routePath, thie means router.resolve is still a heavy operation.

So we should avoid using router-link as it internally calls router.resolve to provide correct link on anchor tag. That is #1353 is still needed.

Though we no longer need RouterLink in that PR, VPLink should still be used by default in markdown and navbar, sidebar links.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@meteorlxy I prefer splitting the VPLink PR by updating the existing one #1353 after you merge this.

@Mister-Hope Mister-Hope changed the title Router feat(client): use visual routes Nov 20, 2023
Comment on lines +2 to +6
const convertedMdPath = path.endsWith('README.md')
? path.substring(0, path.length - 9)
: path.endsWith('.md')
? path.substring(0, path.length - 3) + '.html'
: path
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, if we drop markdown link support, we can have higher performance here.

I would upvote to drop support for markdown links, since we already convert every markdown link in markdown files to <RouterLink> (or <VPLink> with VPLink pr).

At lease we can add an option here?

P.S: What do you think if we add a router field in config files with options like :

router: {
  cleanUrl: true,
  resolveMarkdownLink: true,
  // ...
}

@Mister-Hope Mister-Hope closed this Dec 4, 2023
@Mister-Hope Mister-Hope deleted the router branch December 4, 2023 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] Performance improvements with vue-router
1 participant