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

Support filename that contains non-ASCII and unicode chars #473

Merged
merged 11 commits into from
May 24, 2018

Conversation

ulivz
Copy link
Member

@ulivz ulivz commented May 21, 2018

Background

There is a previous issue of Vue-Router: vuejs/vue-router#838, when using non-ASCII chars as filename, e.g. 尤.md, there will be some issues:

  1. Cannot access directly.
  2. Hit browser back would go to 404.

Fixed

  • Currently VuePress will generate a invalid page component name: page-尤.md:

image

This PR leverage the random hash string prefixed with v- to generate the page component name.

P.S: Testing markdown change will be removed before merged.

Test Coverage

  • Chrome
  • Safari
  • Firefox

To be fixed

Updated: This following issue has been fixed at 9e7a3dc, so you can ignore it.

This PR isn't perfect, which still has an issue, setup to repro:

  1. Open https://deploy-preview-473--vuepress.netlify.com/;
  2. Click navlink at content to go to: https://deploy-preview-473--vuepress.netlify.com/尤.html
  3. Hit browser back button;
  4. Hit browser forward button;
  5. Hit browser back button again, then get 404;

Or:

  1. Open https://deploy-preview-473--vuepress.netlify.com/;
  2. Click navlink to go to: https://deploy-preview-473--vuepress.netlify.com/尤.html
  3. Refresh page
  4. Hit browser back button, then get 404;

I took much time to try to fix it, but still cannot fixed it perfectly.

@yyx990803 Would you please help to review this PR? thanks a lot.

@ulivz ulivz changed the title Support non ascii filename Support filename that contains non-ASCII and unicode chars May 21, 2018
@ulivz ulivz requested a review from yyx990803 May 21, 2018 08:24
@@ -320,7 +322,7 @@ async function genRoutesFile ({ siteData: { pages }, sourceDir, pageFiles }) {
component: ThemeLayout,
beforeEnter: (to, from, next) => {
import(${JSON.stringify(filePath)}).then(comp => {
Copy link
Contributor

Choose a reason for hiding this comment

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

如果 filepath 包含 ! 号,如:/docs/hello!.md,因为使用了 Webpack,! 前面的内容会被认为是 loader 的配置,见:https://webpack.js.org/concepts/loaders/#inline。在我的 PR 中,我使用了 webpack-virtual-modules 解决了这个问题。

Copy link
Member Author

@ulivz ulivz May 21, 2018

Choose a reason for hiding this comment

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

Since webpack does not support the path containing ! and even throw error for that: webpack/webpack#6754 .

So we don't need to introduce webpack-virtual-modules to support ! which should be handled by webpack instead of VuePress.

We don't need to introduce a huge hack to cater to it for an illegal use case.

Copy link
Contributor

@fjc0k fjc0k May 21, 2018

Choose a reason for hiding this comment

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

哈哈,补充下我的思路。

文件:/docs/hello!.md

我们可以在 VuePress 里读取 hello!.md 的内容,并用它生成一个与 hello!.md 同级的 virtual module,比如:docs/page-1.md,然后 import('docs/page-1.md'),如此即可避免 ! 的问题,同时因为是同级目录,原文件里的路径都不用改变。

Copy link
Member Author

@ulivz ulivz May 21, 2018

Choose a reason for hiding this comment

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

我用中文重申一下:我们没有必要为了一个并不合法的极少数用例来引入庞大的代码hack。

Copy link
Contributor

Choose a reason for hiding this comment

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

好吧,我想 ! 仅仅是对于 Webpack 不合法。

Copy link
Member Author

@ulivz ulivz May 21, 2018

Choose a reason for hiding this comment

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

既然 VuePress 基于 Webpack,本着职责清晰的原则,我们不应该在 VuePress 中来解决 Webpack 的问题,万一哪天 Webpack 有自己的解决方案呢?此外,你这种解决方案太复杂了。

如果你需要这个 feature,请给 Webpack 提 Issue 或者 PR,谢谢。

@ulivz ulivz mentioned this pull request May 24, 2018
@ulivz ulivz merged commit 95a3548 into master May 24, 2018
ulivz added a commit that referenced this pull request May 24, 2018
@ulivz ulivz deleted the support-non-ascii-filename branch May 29, 2018 17:22
edm00se added a commit to edm00se/misc-arch that referenced this pull request Jun 6, 2018
@sodatea sodatea mentioned this pull request Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants