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 plugin diagnostic mapping for context="module" #587

Conversation

GrzegorzKazana
Copy link
Contributor

Hi again 👋

After merging fix for the TS diagnostics misplacement within context="module" - #585 , I noticed this issue also occurs for svelte errors/warnings.

Before:
image

After:
image

Changes
Again, moduleScriptInfo was not taken into account when creating SvelteFragmentMapper. Now it is.

Note
In order to prevent similar errors similar to that one, I have scanned the language-server for references to .scriptInfo that are not accompanied by || doc.moduleScriptInfo, and added the missing fallback. While I tried to do my best, these changes were only based on my limited understanding of the codebase, and I would appreciate if someone more experienced could have a look into that.
To keep things clean, the first commit just solves the issue presented on the screen, and the second has the prevention measures (found via grep check, not by any observed issue). If we decide to discard the second commit, I am also fine with that.

P.S.
Going further with diagnostics for context="module", I see that we are not handling correctly cases when component contains both <script>...</script> and <script context="module">...</script>. What are your thoughts on that? Should we wait with merging this, and try to get full handling of that as well at once, or is it fine if we go about it incrementally?

@GrzegorzKazana GrzegorzKazana changed the title Fix svelte plugin diagnostic mapping Fix svelte plugin diagnostic mapping for context="module" Oct 2, 2020
@dummdidumm
Copy link
Member

Thanks a bunch for the fixes! All are legit and will prevent some future inconcistencies.
About your P.S.: Yes I noticed this a while ago, too, but did not get into looking at this more closely. My hope is that this becomes obsolete once preprocessor mapping is handled in svelte core (PR: sveltejs/svelte#5428), so for now I just wait.

@dummdidumm dummdidumm merged commit 6445a5f into sveltejs:master Oct 2, 2020
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