Skip to content

Conversation

@RisaI
Copy link
Contributor

@RisaI RisaI commented Jan 26, 2024

The script regex introduced in c2017e0 matches comment blocks by mistake. This causes files using a header comment tag to always be detected as js. This will produce an error if the script tag imports a module with an explicit .js extensions, where physically only a .ts file is present.

This PR removes the comment matching part of the two regex instances and introduces a test to verify the script language gets properly detected in the presence of a header comment block.

Bug reproduction

<!-- this file uses typescript -->
<script lang="ts">
  // the following will cause Failed to resolve import "./foo.js" from "src/*.svelte". Does the file exist?
  // if physically the file's name is `foo.ts`
  import { foo } from './foo.js';
</script>

@RisaI
Copy link
Contributor Author

RisaI commented Jan 26, 2024

A similar issue might happen here when detecting style blocks:

/<!--[^]*?-->|<style((?:\s+[^=>'"/]+=(?:"[^"]*"|'[^']*'|[^>\s]+)|\s+[^=>'"/]+)*\s*)(?:\/>|>([\S\s]*?)<\/style>)/g;

@dominikg
Copy link
Member

Hi, thank you for this PR.

Unfortunately I think it needs adjustments.

As far as I understand, the comment part of the regex is there to prevent it catching script blocks inside a comment.

<!--
<script lang="ts"> 
// deactivated with a comment, ignore it
</script>
-->
<script>
// active, parse it
</script>

So instead of using the first match, the previous regex should be kept and then matches need to be iterated until the first script match is found.

This isn't ideal but the best we can do for now. Would you be able to make the adjustment?

@RisaI
Copy link
Contributor Author

RisaI commented Jan 27, 2024

Hello, sorry, I didn't realize that was the intent when I first read the code. I added a commented script block to the unit test and made the language detection use matchAll to detect the first match with a capturing group. I reverted the change in error.js, because the presence of the error is checked by the character indices spanned by the matched string, so no false positives should occur.

@dominikg
Copy link
Member

thanks a lot! I added a changeset and this will be released shortly.

@dominikg dominikg merged commit e95d863 into sveltejs:main Jan 27, 2024
@github-actions github-actions bot mentioned this pull request Jan 27, 2024
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.

2 participants