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 #5930

Closed
wants to merge 1 commit into from

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Oct 11, 2022

This PR fixes a bug in our Svelte component tracking implementation where Svelte components without a <script> tag were not tracked.

It adds a function to the preprocessor's markup hook that has access to the whole .svelte component file. In this function we check if there is a script tag and in case there is not, we add an empty <script> </script> tag to the code. This will later on be picked up by the script hook where we inject our trackComponent function call.

Furthermore, this PR adds a few tests to check this new behaviour. It's worth mentioning that I added two "higher level" tests that actually use the Svelte compiler's own preprocess function to which we pass our preprocessor. This gives us a more close to real-use testing scenario.

fixes #5923

expect(preProc.markup).toBeUndefined();
expect(preProc.style).toBeUndefined();
});
describe('script hook', () => {
Copy link
Member Author

@Lms24 Lms24 Oct 11, 2022

Choose a reason for hiding this comment

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

For reviewers: Nothing was changed in the tests within this new describe('script hook') closure. I just added this level because the tests we had previously only test our preprocessor's script hook, while the new tests I added test the markup hook as well as the entire preprocessor

// 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.
// Also, we just want to know if there is a <script> tag in the entire file content; not if
return /<script(\s+.+)?>.*<\/script>/s.test(content);

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data

This [regular expression](1) that depends on [library input](2) may run slow on strings starting with '<script ' and with many repetitions of ' '. This [regular expression](3) that depends on [library input](2) may run slow on strings starting with '<script>' and with many repetitions of 'a>'.
// 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.
// Also, we just want to know if there is a <script> tag in the entire file content; not if
return /<script(\s+.+)?>.*<\/script>/s.test(content);

Check failure

Code scanning / CodeQL

Bad HTML filtering regexp

This regular expression does not match upper case <SCRIPT> tags.
@Lms24
Copy link
Member Author

Lms24 commented Oct 11, 2022

Trying a little different approach at the moment as this regex check is not great

@Lms24 Lms24 marked this pull request as draft October 11, 2022 13:22
@Lms24
Copy link
Member Author

Lms24 commented Oct 12, 2022

Closing this in favour of a new, more robust approach. See #5923 (To Do section) for more details

@Lms24 Lms24 closed this Oct 12, 2022
@AbhiPrasad AbhiPrasad deleted the lms-fix-svelte-cmptrk-no-script branch January 30, 2023 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Svelte Component Tracking doesn't pick up components without a <script> tag
1 participant