Skip to content

Commit 81e4314

Browse files
CopiloteltigerchinoRich-Harris
authored
fix: avoid warning if page options in a Svelte file belongs to a comment (#14180)
* Initial plan * Fix build warning incorrectly triggered by comments mentioning trailingSlash Co-authored-by: eltigerchino <[email protected]> * prettier * refactor: extract comment detection to separate utility file with robust parsing Co-authored-by: Rich-Harris <[email protected]> * rename test file * handle matches in strings * handle template literals, except for super edge case where match occurs inside nested template literal * move to more appropriate location, give function a more appropriate name * changeset --------- Co-authored-by: Copilot <[email protected]> Co-authored-by: eltigerchino <[email protected]> Co-authored-by: Rich Harris <[email protected]> Co-authored-by: Rich-Harris <[email protected]> Co-authored-by: Rich Harris <[email protected]>
1 parent 7e4f858 commit 81e4314

File tree

5 files changed

+264
-1
lines changed

5 files changed

+264
-1
lines changed

‎.changeset/stupid-garlics-roll.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@sveltejs/kit": patch
3+
---
4+
5+
fix: avoid warning if page options in a Svelte file belongs to a comment

‎.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,3 +19,4 @@ vite.config.js.timestamp-*
1919
.test-tmp
2020
symlink-from
2121
.idea/
22+
_tmp_flaky_test_output.txt

‎packages/kit/src/exports/vite/index.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import {
3737
import { import_peer } from '../../utils/import.js';
3838
import { compact } from '../../utils/array.js';
3939
import { build_remotes, treeshake_prerendered_remotes } from './build/build_remote.js';
40+
import { should_ignore } from './static_analysis/utils.js';
4041

4142
const cwd = process.cwd();
4243

@@ -90,7 +91,7 @@ const warning_preprocessor = {
9091
const basename = path.basename(filename);
9192
if (basename.startsWith('+page.') || basename.startsWith('+layout.')) {
9293
const match = content.match(options_regex);
93-
if (match) {
94+
if (match && match.index !== undefined && !should_ignore(content, match.index)) {
9495
const fixed = basename.replace('.svelte', '(.server).js/ts');
9596

9697
const message =

‎packages/kit/src/exports/vite/static_analysis/utils.js

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/**
2+
* Check if a match position is within a comment or a string
3+
* @param {string} content - The full content
4+
* @param {number} match_index - The index where the match starts
5+
* @returns {boolean} - True if the match is within a comment
6+
*/
7+
export function should_ignore(content, match_index) {
8+
// Track if we're inside different types of quotes and comments
9+
let in_single_quote = false;
10+
let in_double_quote = false;
11+
let in_template_literal = false;
12+
let in_single_line_comment = false;
13+
let in_multi_line_comment = false;
14+
let in_html_comment = false;
15+
16+
for (let i = 0; i < match_index; i++) {
17+
const char = content[i];
18+
const next_two = content.slice(i, i + 2);
19+
const next_four = content.slice(i, i + 4);
20+
21+
// Handle end of single line comment
22+
if (in_single_line_comment && char === '\n') {
23+
in_single_line_comment = false;
24+
continue;
25+
}
26+
27+
// Handle end of multi-line comment
28+
if (in_multi_line_comment && next_two === '*/') {
29+
in_multi_line_comment = false;
30+
i++; // Skip the '/' part
31+
continue;
32+
}
33+
34+
// Handle end of HTML comment
35+
if (in_html_comment && content.slice(i, i + 3) === '-->') {
36+
in_html_comment = false;
37+
i += 2; // Skip the '-->' part
38+
continue;
39+
}
40+
41+
// If we're in any comment, skip processing
42+
if (in_single_line_comment || in_multi_line_comment || in_html_comment) {
43+
continue;
44+
}
45+
46+
// Handle escape sequences in strings
47+
if ((in_single_quote || in_double_quote || in_template_literal) && char === '\\') {
48+
i++; // Skip the escaped character
49+
continue;
50+
}
51+
52+
// Handle string boundaries
53+
if (!in_double_quote && !in_template_literal && char === "'") {
54+
in_single_quote = !in_single_quote;
55+
continue;
56+
}
57+
58+
if (!in_single_quote && !in_template_literal && char === '"') {
59+
in_double_quote = !in_double_quote;
60+
continue;
61+
}
62+
63+
if (!in_single_quote && !in_double_quote && char === '`') {
64+
in_template_literal = !in_template_literal;
65+
continue;
66+
}
67+
68+
// If we're inside any string, don't process comment delimiters
69+
if (in_single_quote || in_double_quote || in_template_literal) {
70+
continue;
71+
}
72+
73+
// Check for comment starts
74+
if (next_two === '//') {
75+
in_single_line_comment = true;
76+
i++; // Skip the second '/'
77+
continue;
78+
}
79+
80+
if (next_two === '/*') {
81+
in_multi_line_comment = true;
82+
i++; // Skip the '*'
83+
continue;
84+
}
85+
86+
if (next_four === '<!--') {
87+
in_html_comment = true;
88+
i += 3; // Skip the '<!--'
89+
continue;
90+
}
91+
}
92+
93+
return (
94+
in_single_line_comment ||
95+
in_multi_line_comment ||
96+
in_html_comment ||
97+
in_single_quote ||
98+
in_double_quote ||
99+
in_template_literal
100+
);
101+
}

‎packages/kit/src/exports/vite/static_analysis/utils.spec.js

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
import { expect, test, vi } from 'vitest';
2+
import path from 'node:path';
3+
import { should_ignore } from './utils.js';
4+
5+
// Mock the colors module to avoid issues in tests
6+
vi.mock('kleur', () => ({
7+
default: {
8+
bold: () => ({ red: (/** @type {string} */ str) => str })
9+
}
10+
}));
11+
12+
// We need to test the warning_preprocessor functionality
13+
// Since it's not exported, we'll recreate the relevant parts for testing
14+
const options_regex = /(export\s+const\s+(prerender|csr|ssr|trailingSlash))\s*=/s;
15+
16+
/**
17+
* @param {string} content
18+
* @param {string} filename
19+
*/
20+
function should_warn_for_content(content, filename) {
21+
const basename = path.basename(filename);
22+
if (basename.startsWith('+page.') || basename.startsWith('+layout.')) {
23+
const match = content.match(options_regex);
24+
return match && match.index !== undefined && !should_ignore(content, match.index);
25+
}
26+
return false;
27+
}
28+
29+
test.each([
30+
[
31+
'single-line comment with export const trailingSlash',
32+
'// export const trailingSlash = "always"',
33+
'+page.svelte',
34+
false
35+
],
36+
[
37+
'multi-line comment with export const trailingSlash',
38+
'/* export const trailingSlash = "always" */',
39+
'+page.svelte',
40+
false
41+
],
42+
[
43+
'HTML comment with export const trailingSlash',
44+
'<!-- export const trailingSlash = "always" -->',
45+
'+page.svelte',
46+
false
47+
],
48+
[
49+
'single-line comment with export const ssr',
50+
'// export const ssr = false',
51+
'+page.svelte',
52+
false
53+
],
54+
[
55+
'valid export const trailingSlash',
56+
'export const trailingSlash = "always"',
57+
'+page.svelte',
58+
true
59+
],
60+
['valid export const ssr', 'export const ssr = false', '+page.svelte', true],
61+
[
62+
'valid export const with spacing',
63+
'export const trailingSlash = "always"',
64+
'+page.svelte',
65+
true
66+
],
67+
[
68+
'valid export on non-page file should not warn',
69+
'export const trailingSlash = "always"',
70+
'regular.svelte',
71+
false
72+
],
73+
[
74+
'comment followed by valid export',
75+
'// This is a comment\nexport const trailingSlash = "always"',
76+
'+page.svelte',
77+
true // Should still warn for the valid export
78+
],
79+
[
80+
'valid export with trailing comment',
81+
'export const trailingSlash = "always" // with comment after',
82+
'+page.svelte',
83+
true
84+
],
85+
[
86+
'comment with */ inside - not actually nested',
87+
'/* comment with */ inside */ export const trailingSlash = "always"',
88+
'+page.svelte',
89+
true // This should warn because the export is not in a comment
90+
],
91+
[
92+
'multiple line comment spanning lines',
93+
'/*\n * multi-line comment\n * export const trailingSlash = "always"\n */',
94+
'+page.svelte',
95+
false
96+
],
97+
[
98+
'multiple single-line comments',
99+
'// first comment\n// export const trailingSlash = "always"\n// third comment',
100+
'+page.svelte',
101+
false
102+
],
103+
[
104+
'HTML comment spanning multiple lines',
105+
'<!--\n export const trailingSlash = "always"\n-->',
106+
'+page.svelte',
107+
false
108+
],
109+
[
110+
'edge case: comment delimiters in strings',
111+
'"/*"; export const trailingSlash = "always"; "*/"',
112+
'+page.svelte',
113+
true // Should warn because the export is not actually in a comment
114+
],
115+
[
116+
'escaped quotes in strings',
117+
'"comment with \\"/*\\" inside"; export const trailingSlash = "always"',
118+
'+page.svelte',
119+
true
120+
],
121+
[
122+
'single quotes with comment delimiters',
123+
"'/*'; export const trailingSlash = 'always'; '*/'",
124+
'+page.svelte',
125+
true
126+
],
127+
[
128+
'template literal with comment delimiters',
129+
'`/*`; export const trailingSlash = "always"; `*/`',
130+
'+page.svelte',
131+
true
132+
],
133+
[
134+
'real comment after string with comment delimiter',
135+
'"fake /*"; /* real comment with export const trailingSlash = "always" */',
136+
'+page.svelte',
137+
false
138+
],
139+
[
140+
'nested comment-like structures in strings',
141+
'"/* /* nested */ */"; export const trailingSlash = "always"',
142+
'+page.svelte',
143+
true
144+
],
145+
['page option match inside string', '"export const trailingSlash = true"', '+page.svelte', false],
146+
[
147+
'page option match inside template literal',
148+
'`${42}export const trailingSlash = true`',
149+
'+page.svelte',
150+
false
151+
]
152+
])('warning behavior: %s', (_description, content, filename, should_warn) => {
153+
const result = should_warn_for_content(content, filename);
154+
expect(result).toBe(should_warn);
155+
});

0 commit comments

Comments
 (0)