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

fix: nested comments and strings, new regexp utils #7650

Merged
merged 27 commits into from Apr 11, 2022

Conversation

poyoho
Copy link
Member

@poyoho poyoho commented Apr 8, 2022

Description

fix: #7632
fix: #7647
mentioned #7549

Additional context

fix comment nested string, string nested comment

// "

const a = 1

// "
console.log("2 //", new URL("./logo.svg", import.meta.url).href);

and try to create regexes general utilities #7549


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@patak-dev patak-dev added the p3-minor-bug An edge case that only affects very specific usage (priority) label Apr 8, 2022
@patak-dev patak-dev added this to the 3.0 milestone Apr 8, 2022
@patak-dev patak-dev changed the title chore: regexp fix: nested comments and strings, new regexp utils Apr 8, 2022
@poyoho
Copy link
Member Author

poyoho commented Apr 8, 2022

If ok I'll apply util to the modules mentioned by #7549 😊

@patak-dev
Copy link
Member

I think it is ok to have this refactoring in a single PR, yes. I like that the regexes were combined in a single one. About the utils, maybe CleanString shouldn't extend String? Now you have the string itself, raw and clean. I think I would find it less confusing to use a simpler type { raw: string, clear: string } or something like this, and regular free functions to pass it around.

@poyoho
Copy link
Member Author

poyoho commented Apr 9, 2022

@patak-dev how about this. I think findEmptyStringRawIndex also can handle in walkCleanString. I don't know if it should continue to encapsulate like this.

if(s[0] === '`' || s[0] === '"' || s[0] === "'") {
 const [start, end] = findEmptyStringRawIndex(cleanString, s, index)
 match[xxx] = cleanString.raw.slice(start, end)
}

@poyoho
Copy link
Member Author

poyoho commented Apr 9, 2022

I think it is ok to have this refactoring in a single PR

If need a new PR, I think should merge this PR first. 👀 #7549 dependence on CleanString

But there is a known bug that has not been resolved that the nested string template syntax does not match. Since the previous regexp match should just ignore the comment, adding a string template might have a destructive error, since most of the time all the code is processed.

@poyoho
Copy link
Member Author

poyoho commented Apr 10, 2022

maybe string template don't need to replace all. #7647
just need to

const str = `${a${`c${`d`}`}b}`

const clean = `${\0${`\0${`\0`}`}\0}`

@poyoho
Copy link
Member Author

poyoho commented Apr 10, 2022

Because regular expressions can't be written, we can only use lex methods such as import.meta.glob. If this method is acceptable, even handle string and comment in lex 🙆‍♂️

@patak-dev
Copy link
Member

patak-dev commented Apr 10, 2022

@poyoho interesting. Using a lexer only for template strings may be fast enough 🤔
What do you think about separating this into two PRs though? I think we could merge the cleanup and fix for comments and strings if we get others to review. And then we can keep discussing about the new template string lexer in another PR to avoid blocking this one.

Note: if you think it is hard to divide it in two, then let's keep it together and ask for others to check it out.

@poyoho
Copy link
Member Author

poyoho commented Apr 11, 2022

It's okay, it's easy

packages/vite/src/node/cleanString.ts Outdated Show resolved Hide resolved
packages/vite/src/node/cleanString.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/assetImportMetaUrl.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/assetImportMetaUrl.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/workerImportMetaUrl.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/workerImportMetaUrl.ts Outdated Show resolved Hide resolved
packages/vite/src/node/cleanString.ts Outdated Show resolved Hide resolved
bluwy
bluwy previously approved these changes Apr 11, 2022
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Thanks for handling these bugs :)

@patak-dev patak-dev merged commit 93900f0 into vitejs:main Apr 11, 2022
@patak-dev patak-dev removed this from the 3.0 milestone Apr 11, 2022
@poyoho poyoho deleted the chore/regexp branch April 11, 2022 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
3 participants