-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(svelte): Track components without <script> tags #5957
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
Changes from all commits
e11dbfe
731cdc0
157f8ea
ed12846
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,8 +22,30 @@ export function componentTrackingPreprocessor(options?: ComponentTrackingInitOpt | |
const mergedOptions = { ...defaultComponentTrackingOptions, ...options }; | ||
|
||
const visitedFiles = new Set<string>(); | ||
const visitedFilesMarkup = new Set<string>(); | ||
|
||
const preprocessor: PreprocessorGroup = { | ||
// This markup hook is called once per .svelte component file, before the `script` hook is called | ||
// We use it to check if the passed component has a <script> tag. If it doesn't, we add one to inject our | ||
// code later on, when the `script` hook is executed. | ||
markup: ({ content, filename }) => { | ||
const finalFilename = filename || 'unknown'; | ||
const shouldInject = shouldInjectFunction(mergedOptions.trackComponents, finalFilename, {}, visitedFilesMarkup); | ||
|
||
if (shouldInject && !hasScriptTag(content)) { | ||
// Insert a <script> tag into the component file where we can later on inject our code. | ||
// We have to add a placeholder to the script tag because for empty script tags, | ||
// the `script` preprocessor hook won't be called | ||
// Note: The space between <script> and </script> is important! Without any content, | ||
// the `script` hook wouldn't be executed for the added script tag. | ||
const s = new MagicString(content); | ||
s.prepend('<script>\n</script>\n'); | ||
return { code: s.toString(), map: s.generateMap().toString() }; | ||
} | ||
|
||
return { code: content }; | ||
}, | ||
|
||
// This script hook is called whenever a Svelte component's <script> content is preprocessed. | ||
// `content` contains the script code as a string | ||
script: ({ content, filename, attributes }) => { | ||
|
@@ -99,3 +121,18 @@ function getBaseName(filename: string): string { | |
const segments = filename.split('/'); | ||
return segments[segments.length - 1].replace('.svelte', ''); | ||
} | ||
|
||
function hasScriptTag(content: string): boolean { | ||
// This regex is taken from the Svelte compiler code. | ||
// They use this regex to find matching script tags that are passed to the `script` preprocessor hook: | ||
// https://github.com/sveltejs/svelte/blob/bb83eddfc623437528f24e9fe210885b446e72fa/src/compiler/preprocess/index.ts#L144 | ||
// However, we remove the first part of the regex to not match HTML comments | ||
const scriptTagRegex = /<script(\s[^]*?)?(?:>([^]*?)<\/script\s*>|\/>)/gi; | ||
Check failureCode scanning / CodeQL Bad HTML filtering regexp
This regular expression does not match script end tags like </script\t\n bar>.
|
||
|
||
// Regex testing is not a super safe way of checking for the presence of a <script> tag in the Svelte | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree this is a bit sketchy. Non blocking but have you looked at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was just thinking about that too on my way to the train (apparently, walking really helps some time 😅). Will take a look and see if anything comes out of it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems much more elegant and safer to use svelte's parser. Changed it in 31f1c52. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Amazing. Super clean! |
||
// component file but I think we can use it as a start. | ||
// A case that is not covered by regex-testing HTML is e.g. nested <script> tags but I cannot | ||
// think of why one would do this in Svelte components. For instance, the Svelte compiler errors | ||
// when there's more than one top-level script tag. | ||
return scriptTagRegex.test(content); | ||
} |
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.
Just out of curiosity: Do you know why they have multiline comments like that in their regex?
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.
I was also wondering about that. Traced it back yesterday to a commit saying that this helps them actually getting rid of comments later on. I didn't reach a point where I was entirely convinced but i suspect that it has something to do with how they process the "replacements" they identify, leading them to do nothing if there is nothing to preprocess here