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

$site referencing to siteDataByRoute #159

Closed
patak-dev opened this issue Nov 26, 2020 · 5 comments
Closed

$site referencing to siteDataByRoute #159

patak-dev opened this issue Nov 26, 2020 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@patak-dev
Copy link
Member

Spawn from a discussion in #152

About siteDataByRoute, if $title and $description are localized, I think $site should also be localized pointing to siteDataByRoute instead of siteData. I think that users accessing $site from the markdown, should really be looking for $siteDataByRoute.

In enhanceApp siteData actually means siteDataByRoute: https://github.com/vuejs/vitepress/blob/master/src/client/app/index.ts#L118

export default {
  Layout,
  NotFound: () => 'custom 404',
  enhanceApp({ app, router, siteData }) {
     // `siteData`` is a `ref`` of current site-level metadata. <- this is really siteDataByRoute
  }
}

So it makes sense for $site to follow suit.

We will be breaking compat slightly here with vuepress, but I think this is a good change that will help avoid i18n issues and it will be really confusing that siteData from enhanceApp is not the same as $siteData in vitepress.

If we do this change, we could expose unlocalized siteData with another name. Maybe $unlocalizedSite?

@kiaking kiaking added the bug Something isn't working label Nov 27, 2020
@kiaking
Copy link
Member

kiaking commented Nov 27, 2020

You have very good points. And I too am very tempted to make $site to be $siteByRoute. However, I would still say we wanna go with $site and $siteByRoute approach.

Unlike $title and $description, $site really is, in most of the case, the .vitepress/config.js. Best thing about this is that it's very intuitive. No confusion. It's more convenient to have $siteByRoute, if you know VitePress well, but I think getting the whole config.js from $site will not surprise users.

They can still handle i18n logics using the $site, because all of the settings are there. And when they find out there's $siteByLocale, it will be pure enhancement for them. For me, $title and $description is the same "helper functions". It's not about config, but convenience shortcut for users.

So $site is, as its name stand for, the whole site data. And I'm very open to add more helpers to make getting particular site configs.

For example, another consistency issue I think we could have is, multi sidebar settings. When you're on /guide/ path, should $site only contain sidebar configs for /guide/ path? Maybe yes, maybe not, but if $site is locale based, why not? Right? We can avoid this inconsistency discussion, and still be helpful, by introducing $siteByPath. And who knows what other similar cases we could have.

Therefore, I think being "opt-in" to something is better than "opt-out". Where we have bare basic $site that returns everything, then we have more helper function to help users.

We should definitely change enhanceApp, we should probably provide everything there.

export default {
  Layout,
  NotFound: () => 'custom 404',
  enhanceApp({ app, router, siteData, siteDataByRoute, pageData }) {
    // ...
  }
}

P.S.
I think it would have been great if the $site was named $config, or something. So it was easier to have $site as current side config 😅

@patak-dev
Copy link
Member Author

I thought the same about $config, I even searched for $siteConfig to see if we could that name but it is being used for vitepress config.

I understand your point and I agree that it is better to keep $site aligned with Vuepress, given that siteData in enhanceData is also pointing to the same thing.

Maybe it is just a matter of finding a good name for $siteByRoute. Since $page is localized (and we will point users to $page.title for example) maybe we should start from there instead. So $page.config is the localized and by-path version of the site config. It is the site config as viewed from the current page (and $pageConfig could be provided, or maybe just $config 🤔 ). But $page.config.title doesn't read well as the site title.
Maybe following the decision to make $page.title localized, we could also make $page.site localized so that is what points to siteByRoute.
Other options: $routeConfig, $pathConfig or expose all this info in $route.config.

But this is bikeshedding already, thanks for the detailed explanation again. And I think this issue is solved if enhanceData is coherent with $site 👍

@kiaking
Copy link
Member

kiaking commented Nov 27, 2020

Maybe it is just a matter of finding a good name

Yeah I'm very open to discuss this. I think putting under $page might make sense, but not so sure. Since $page is really the page it self, and nothing more I think. And I'm talking page I mean the markdown contents. So even you remove sidebars, navbars and everything, page should have no difference. It's a page.

Where else, site is a whole site, even though it is localized. It contains sidebar, navbar, which page do not care. Again, if we were calling the $page to be $content, for example, maybe it would have made sense to name $siteByLocale to be $page 🤔

And while I was trying to come up with different names, I also felt like maybe it's going to be confusing to have almost identical stuff in a different name, or in different place. $site and $page.site is same thing (without i18n), but it's not top down you know what I mean. It's different from having $site.themeConfig and $themeConfig, where it's just a shortcut.

At least, I would like to change it to $siteByLocale, instead of $siteByRoute though. Technically $siteByRoute is more correct in terms of implementation, because it's really a route (path) checking, but as a domain point of view, it's locale specific feature so 😳

@patak-dev
Copy link
Member Author

I have the same feeling about $title and $site.title being different. I think is an ok price to pay for vuepress compat though.

Agree with $page. I think I also thought about it because there are already configs at the $page level for how the site looks. For example: $page.frontmatter.sidebar false to hide the sidebar for this page (same with navbar). $site, $page, $content would have been quite nice. I thought about $site, $document, $page in a similar line (but discarded it because $document.title will not be the document title). Naming is hard 😄

@kiaking
Copy link
Member

kiaking commented Nov 27, 2020

Very good point. Yeah the $title should have been $documentTitle really 😅 , Well it's "site's title", "page's title" so the name is same, but technically different thing (I'm just telling it to convince my self here 🏃)

For now, I'll fix the inconsistency to close this issue. Let's think a bit more about the naming of $siteByRoute before we expose it.

@kiaking kiaking self-assigned this Nov 27, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants