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: support theme default config and extra pages rendering #315

Closed

Conversation

meteorlxy
Copy link
Member

@meteorlxy meteorlxy commented May 1, 2018

Three features:

  1. Support theme default config by theme-dir/config.js
  2. Allow custom theme to render extra pages (#198) (Another implementation Feature: theme-related pages #313)
  3. Allow user to render extra pages based on a theme (Dynamic routes do not get generated with build #160)

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 config

To achieve Feature 2, add extraPages in theme-dir/config.js, and add coresponding extra routes in theme-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's extraPages config could be overwritten in .vuepress/config.js.

E.g:

// .vuepress/config.js
// or theme-dir/config.js
module.exports = {
  extraPages: [
    '/test'
  ],
  // other config...
}
// .vuepress/enhanceApp.js
// or theme-dir/enhanceApp.js
const Test = () => import("./Test");

export default ({ Vue, options, router }) => {
  router.addRoutes([
    { path: "/test", component: Test }
  ])
}

Docs will be added after accepted.

@meteorlxy
Copy link
Member Author

What do you think about this? @mdaffin

Copy link
Contributor

@ycmjason ycmjason left a 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 = {}
Copy link
Contributor

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 {}

Copy link
Member Author

@meteorlxy meteorlxy May 3, 2018

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.

Copy link
Contributor

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.

@ycmjason
Copy link
Contributor

ycmjason commented May 3, 2018

Another note, I see there is a lot in the prepare.js

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?

@meteorlxy
Copy link
Member Author

meteorlxy commented May 5, 2018

@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 meteorlxy changed the title feat: support extra pages rendering feat: support theme default config and extra pages rendering May 5, 2018
@ycmjason
Copy link
Contributor

ycmjason commented May 6, 2018

@meteorlxy
The current implementation make sure one file is rendered after another. I wonder if this is intended. We could have used await Promise.all(promises) instead of for-await. Let me try it on my local and submit a PR regarding this.

@meteorlxy
Copy link
Member Author

@ycmjason Yeah, I also thought about Promise.all but didn't try it 😄

@ycmjason
Copy link
Contributor

ycmjason commented May 7, 2018

@meteorlxy
9719bc3

Looks like the for-await loop is intentional. I wonder why tho.

@ycmjason
Copy link
Contributor

ycmjason commented May 7, 2018

I see, this is probably for better CLI output. The time is not much different so Evan chose to use for-await probably..

@Feofilakt
Copy link

Why is it closed?

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.

4 participants