-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: avoid warning if page options in a Svelte file belongs to a comment #14180
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
Conversation
|
@eltigerchino 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
🦋 Changeset detectedLatest commit: 1e6c0ed The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…ngSlash Co-authored-by: eltigerchino <[email protected]>
| * @param {number} matchIndex - The index where the match starts | ||
| * @returns {boolean} - True if the match is within a comment | ||
| */ | ||
| function isWithinComment(content, matchIndex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't duplicate the functions here. You should export a utility function that checks the content and re-use the function between the test and the original file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! I've extracted the is_within_comment function to a new comment_utils.js file and updated both the main code and tests to use the exported utility. The function is no longer duplicated. Commit: f29b350
|
@copilot a few things:
|
…ust parsing Co-authored-by: Rich-Harris <[email protected]>
All feedback addressed in commit f29b350: ✅ Extracted comment detection to new The new implementation properly handles strings, escape sequences, and all comment types while correctly identifying when exports are truly within comments vs. when comment delimiters appear in strings. |
The build process was incorrectly triggering warnings when comments in
.sveltefiles mentionedexport const trailingSlashor other page options. This happened because the regex pattern used to detect page option exports didn't account for JavaScript/HTML comments.For example, this code would incorrectly trigger a warning:
The warning would show:
This fix adds an
is_within_comment()helper function in a newcomment_utils.jsfile that robustly checks if regex matches occur within://)/* */)<!-- -->)The implementation properly handles edge cases like comment delimiters appearing within strings (e.g.,
"/*"; export const trailingSlash = "always"; "*/") by tracking string boundaries and escape sequences.The solution is minimal and surgical - it preserves the existing regex but adds a validation step to exclude matches found within comments. All existing functionality remains unchanged while eliminating false positive warnings.
Fixes #13781.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.