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: correctly detect real Svelte files for declarationMap #2443

Merged
merged 6 commits into from
Jul 18, 2024

Conversation

ryanatkn
Copy link
Contributor

@ryanatkn ryanatkn commented Jul 18, 2024

I opened #2442 - declarationMap "sources" being incorrect for .svelte.ts and .svelte.js files - and this appears to fix it.

I struggled with the tests, I'm sorry for not getting them right first. It was working in my reproduction but the paths are different in the tests because of pathPrefix, and moving the check after sourcePath is adjusted for pathPrefix seems to be correct both in the tests here and my bug reproduction.

@@ -9,7 +9,8 @@
"esModuleInterop": true,
"allowJs": true,
"checkJs": true,
"declarationMap": true
"declarationMap": true,
"declaration": true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TypeScript complains when declarationMap is enabled without declaration, the other tsconfig with declarationMap enabled in language-server has it already.

// Due to our hack of treating .svelte files as .ts files, we need to adjust the extension
if (svelteMap.get(path.join(options.rootDir, toRealSvelteFilepath(sourcePath)))) {
sourcePath = toRealSvelteFilepath(sourcePath);
}
Copy link
Contributor Author

@ryanatkn ryanatkn Jul 18, 2024

Choose a reason for hiding this comment

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

If this code isn't moved after the sourcePath inversion, it fails the lookup to see if it's a real Svelte file.

The first line is the fix, and the second line is just cleanup to use an existing helper for clarity.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Thank you!

@dummdidumm dummdidumm merged commit 527c2ad into sveltejs:master Jul 18, 2024
3 checks passed
dummdidumm added a commit that referenced this pull request Jul 30, 2024
bug introduced in #2443, affecting windows systems
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