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

Align param source priority for theme with other params (cliOptions > siteConfig) #1116

Closed

Conversation

juno-yu
Copy link

@juno-yu juno-yu commented Dec 18, 2018

Summary
For all other parameters cliOptions has higher priority than siteOptions,
for theme it is the opposite.
When command line arguments are not of highest priority here,
Some strange behavior might come up for usages.

packages/@vuepress/core/lib/prepare/CacheLoader.js
20:  let cache = cliOptions.cache || siteConfig.cache || defaultCacheDirectory

packages/@vuepress/core/lib/prepare/AppContext.js
51:    const { tempPath, writeTemp } = createTemp(cliOptions.temp)
77:    const rawOutDir = this.cliOptions.dest || this.siteConfig.dest
168:    this.pluginAPI.useByPluginsConfig(this.cliOptions.plugins)

packages/@vuepress/core/lib/prepare/loadTheme.js
33:  const theme = siteConfig.theme || cliOptions.theme

packages/@vuepress/core/lib/dev.js
79:  const port = await resolvePort(cliOptions.port || ctx.siteConfig.port)
80:  const { host, displayHost } = await resolveHost(cliOptions.host || ctx.siteConfig.host)
124:    open: cliOptions.open,

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI of default theme, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature
  • Related documents have been updated
  • Related tests have been updated

@juno-yu juno-yu changed the title Align params source order for theme with other params (cliOptions > siteConfig) Align param source priority for theme with other params (cliOptions > siteConfig) Dec 18, 2018
@ulivz
Copy link
Member

ulivz commented Dec 18, 2018

What are the some strange behaviors that might come up?

For now cliOptions wasn't completely retrieved from user's cli options. and --theme wasn't a cli flag

@ulivz ulivz closed this Dec 18, 2018
@juno-yu
Copy link
Author

juno-yu commented Dec 18, 2018

Sorry, what i mean is option specified like

vuepress.build(sourceDir, {
  theme: 'some option'
})

would always be ignored whenever there is corresponding such key in vuepressDir/config.{js|yml|toml}.

This is a programming usage which one cannot modify the files in original directory but need to use a different theme.
this parameter parsing looks different from other parameter parsing?

@ulivz
Copy link
Member

ulivz commented Dec 18, 2018

Sorry but for now we didn't recommend any programmable usage for now.

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.

2 participants