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(svelte): Track components without <script> tags #5957

Merged
merged 4 commits into from Oct 14, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 37 additions & 0 deletions packages/svelte/src/preprocessors.ts
Expand Up @@ -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 }) => {
Expand Down Expand Up @@ -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
Copy link
Member

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?

Copy link
Member Author

@Lms24 Lms24 Oct 14, 2022

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

// However, we remove the first part of the regex to not match HTML comments
const scriptTagRegex = /<script(\s[^]*?)?(?:>([^]*?)<\/script>|\/>)/gi;
github-advanced-security[bot] marked this conversation as resolved.
Fixed
Show resolved Hide resolved

// Regex testing is not a super safe way of checking for the presence of a <script> tag in the Svelte
Copy link
Member

Choose a reason for hiding this comment

The 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 import { parse } from 'svelte/compiler'; if it outputs something we could use that is a little bit more robust?

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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);
}