-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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: support theme default config and extra pages rendering #315
feat: support theme default config and extra pages rendering #315
Conversation
What do you think about this? @mdaffin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just my opinion. It is not something you have to change.
lib/prepare.js
Outdated
delete require.cache[configPath] | ||
|
||
// resolve siteConfig | ||
let config = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just my honest opinion 😊❤️🙈, I think 335-344 would be cleaner with the following:
if (fs.existsSync(configYmlPath)) {
return await parseConfig(configYmlPath)
}
if (fs.existsSync(configTomlPath)) {
return await parseConfig(configTomlPath)
}
if (fs.existsSync(configPath)) {
return require(configPath)
}
return {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh looks better. I just moved things down into a function without much refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea yea! Just spot that when I see the changes, might be worth it to make the change.
Another note, I see there is a lot in the for (file of ...) {
await render(file)
} Is this intentional? Since it looks like this will slow down the prepare process a lot? Or the rendering needs to be done sequentially? |
@ycmjason Maybe we should ensure all the files have been rendered? Or some of the pages may not be rendered before the script exits? |
@meteorlxy |
@ycmjason Yeah, I also thought about |
Looks like the for-await loop is intentional. I wonder why tho. |
I see, this is probably for better CLI output. The time is not much different so Evan chose to use for-await probably.. |
14d9013
to
c7c0cd9
Compare
c992e38
to
c2eaff3
Compare
0c3bc3a
to
cf1e6fc
Compare
Why is it closed? |
Three features:
theme-dir/config.js
Feature 1 is necessary for Feature 2. I propose to use
theme-dir/config.js
, which is similar to.vuepress/config.js
, as the theme default configTo achieve Feature 2, add
extraPages
intheme-dir/config.js
, and add coresponding extra routes intheme-dir/enhanceApp.js
. That will enable extra pages rendering for theme.To achieve Feature 3, add
extraPages
in.vuepress/config.js
, and add coresponding extra routes in.vuepress/enhanceApp.js
. That will enable extra pages rendering based on a theme. Notice that theme'sextraPages
config could be overwritten in.vuepress/config.js
.E.g:
Docs will be added after accepted.