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

Change regex to avoid lookbehind #323

Merged
merged 3 commits into from
May 2, 2018
Merged

Change regex to avoid lookbehind #323

merged 3 commits into from
May 2, 2018

Conversation

whoan
Copy link
Contributor

@whoan whoan commented May 2, 2018

An alternative to PR #321.
Also related to PR #308 to fix issue #306.

This is to avoid updating node to 8.10 to support Regex lookbehind.

@meteorlxy
Copy link
Member

I think it's buggy.

Have you tested?

@meteorlxy
Copy link
Member

I have ever used this in #308:

const indexRE = /(^|\/)(index|readme)\.md$/i

    // README.md -> /
    // foo/README.md -> /foo/
    const fileReplace = file.replace(indexRE, '')
    return '/' + ( fileReplace === '' ? '' : fileReplace + '/')

But I prefer to change a single line, so I chose lookbehind.

@whoan
Copy link
Contributor Author

whoan commented May 2, 2018

I tested only with this input and it worked:

'/foo/Gitter-Room-Index.md' -> '/foo/Gitter-Room-Index.md'
'/Gitter-Room-Index.md' -> '/Gitter-Room-Index.md'
'Gitter-Room-Index.md' -> 'Gitter-Room-Index.md'
'README.md' -> '/'
'foo/README.md' -> '/foo/'
'/foo/README.md' -> '/foo/'

@meteorlxy
Copy link
Member

meteorlxy commented May 2, 2018

@whoan I tested your code in origin/master but errors occurred.

You can consider my code above, which I had tested well.

@whoan
Copy link
Contributor Author

whoan commented May 2, 2018

@meteorlxy I had forgotten to remove a (now) duplicated slash because I am appending it in the replacement.

@meteorlxy
Copy link
Member

meteorlxy commented May 2, 2018

@whoan
I suggest you to test in official mater branch first.

Yours:

  • 'foo/bar/guide/Readme.md' => '/guide/'

Expected:

  • 'foo/bar/guide/Readme.md' => '/foo/bar/guide/'

Copy link
Member

@meteorlxy meteorlxy left a comment

Choose a reason for hiding this comment

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

Change suggested

lib/prepare.js Outdated
@@ -236,14 +236,14 @@ async function genComponentRegistrationFile ({ sourceDir }) {
return `import Vue from 'vue'\n` + components.map(genImport).join('\n')
}

const indexRE = /(?<=(^|\/))(index|readme)\.md$/i
const indexRE = /^\/?([^/]+\/)*(index|readme)\.md$/i
Copy link
Member

Choose a reason for hiding this comment

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

This may help:

const indexRE = /(^\/?([^/]+\/)*)(index|readme)\.md$/i

Copy link
Member

Choose a reason for hiding this comment

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

And maybe this is enough:

const indexRE = /(^|.*\/)(index|readme)\.md$/i

Copy link
Member

Choose a reason for hiding this comment

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

The second one looks good, I just tested for it, so let me leverage it.

lib/prepare.js Outdated
@@ -236,14 +236,14 @@ async function genComponentRegistrationFile ({ sourceDir }) {
return `import Vue from 'vue'\n` + components.map(genImport).join('\n')
}

const indexRE = /(?<=(^|\/))(index|readme)\.md$/i
const indexRE = /^\/?([^/]+\/)*(index|readme)\.md$/i
Copy link
Member

Choose a reason for hiding this comment

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

The second one looks good, I just tested for it, so let me leverage it.

@ulivz ulivz merged commit 657d341 into vuejs:master May 2, 2018
@whoan whoan deleted the regex branch May 2, 2018 16:47
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.

3 participants