fix: missing js sourcemaps with rewritten imports broke debugging (#7767)

[BenceSzalai]

Description

Supposedly fixes #7767

refs vitejs/vite-plugin-react#23 #5834

Additional context

See detailed discussion under the related issue

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes vitejs/vite#123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[sapphi-red]

I tested with VSCode but this didn't work. Does this work with WebStorm or other IDE/code editors?

[BenceSzalai]

I'm not using VSCode, so I did not test it before, but I did now compare the behaviour with PhpStorm (which will serve as my benchmark for all InteliJ IDEs).

What works in both:

• The initial version of the file modified by Vite is resolved to the original source file, breakpoint is hit AND shown in the source file without opening the file in the form seen by the Browser.

Where things divert is:

• With HMR enabled if a change is made to the file a new version is loaded with a ?t=... parameter.
  • In PhpStorm the breakpoint is again hit normally and correctly resolved to the original source file as well without opening up the actual version of the file served by Vite dev mode (rewritten import).
  • In VSCode this only kind-of works. The good news is that the breakpoint is hit. However VSCode first opens up the version of the file as seen by the Browser (rewritten import and the sourcemap added) showing a breakpoint being hit. Once the command to continue execution is given now the original source file is shown with the same breakpoint being hit in it. So I'd say it's a bit inconvenient but still better than without the sourcemaps.

Long story short I think from this point this issue should be raised with VSCode team. They have to look into why VSCode cannot understand that now a new file seen by the Browser is mapped to the same source as another file was before and why they show the same breakpoint being hit twice in two different versions of the files.

My assumption is that it is an IDE issue, whereas as suggested by JetBrains the issue effecting InteliJ IDEs was that the sourcemaps were missing and with that fixed it works fine in PhpStorm (and we can assume in their other IDEs as well).

EDIT:

Additional notes

I've found that if the debugging browser is closed and opened again VSCode resolves to the correct file once again without Vite dev restarted, so it is kind of like an issue between the broswer and the IDE agreeing on which file seen by the browser resolves to which file seen by the IDE. Not much to do with Vite.

Maybe as a workaround Vite could force the browser to not only load the new version of the file under a new URL, but also to load e.g. an empty file from the previous URL so there would be no more than one file at one time being resolved to the same source. But that would be kind of like adapting Vite to a bug in VSCode. Would not hurt probably, but it really should be a fix in the way the IDE interprets the information from the Browser.

Still I think the fact that Browser debugging in IDE works fine before HMR with this patch both in VSCode and InteliJ is a nice step forward.
Browser debugging not working after HMR of a file with a breakpoint is a more marginal issue, and IDE specific to VSCode.

PS: Did not try in other IDEs, I don't even know others. 🤷‍♂️

[BenceSzalai]

I tested with VSCode but this didn't work. Does this work with WebStorm or other IDE/code editors?

@sapphi-red: Could you tell more about what is not working for you? Probably it is something other than the issues I've found it may worth looking into more...

I've found another interesting aspect: If imports in the original source are spread to multiple lines, it may interfere with this solution, but I'll check that and probably even if this is the case the solution would be to simply keep imports in as many lines as they are in the source so we could still go ahead with low cost dummy sourcemaps while at the same time hopefully fix this kind of problem as well. Will try to reproduce, examine and report back soon.

[sapphi-red]

Thank you for testing it and the detailed report!

However VSCode first opens up the version of the file as seen by the Browser (rewritten import and the sourcemap added) showing a breakpoint being hit.

This was what I was talking about.

I was testing around VSCode's debugger with Vite and found that setting enableContentValidation: false makes it work without this PR.

{
	"version": "0.2.0",
	"configurations": [
		{
			"name": "Launch Chrome",
			"request": "launch",
			"type": "pwa-chrome",
			"url": "http://localhost:5173",
			"webRoot": "${workspaceFolder}",
			"enableContentValidation": false
		}
	]
}

I think this solution is most ideal. This way Vite won't need to change anything and of course it won't have any perf implications.
Is there any option like this in InteliJ IDEs?

[PotRemus]

Hello,
Do you know if this pull request has a chance of passing? and for when?
Indeed, with the enableContentValidation option bypasses the concern for vs code, it is not practical for other ides (ex: Visual Studio 2022) or chrome DevTools.
Even if it is an option in vite config, so as not to impact performance for users not affected by this problem.
Thanks

[BenceSzalai]

Is there any option like this in InteliJ IDEs?

I don't think so. They apparently wish to stick to the standard on this one: if a file is served from a location that the IDE cannot correspond with the development folder, because they have different contents, they expect a SourceMap to be present.

See the related issue here: https://youtrack.jetbrains.com/issue/WEB-55544

Basically it is a "won't fix" / "feature request" to allow different files to be debugged as if they were the same.

This way Vite won't need to change anything and of course it won't have any perf implications.

Since Vite rewrites imports, and imports may be formatted in source code to spread multiple lines a proper SourceMap will still be needed even in VSCode in those cases, because as soon as the line numbers shift (as Vite rewrites the imports) a simple file path mapping won't be enough for proper debugging. I know I said moths ago I'll check this, but had other things to do. Now I've managed to get back on this, so will look into it.

Regarding performance we could make this optional. Something like a new setting under Server Options called sourcemap. However we have to consider that this whole thing we are talking about here only happens for files where pluginContainer.transform() call did not return a sourceMap. So basically it boils down to that Vite adds full blown source maps to several files*, and we only create additional sourcemaps for *.js files where they were not created by transform plugins. I don't expect this to cause perceivable issues, as it is only part of the files being processed. But if the concern is real, we can add an option. But that option should probably have a more explicit name, as simple sourcemap would suggest one could turn of sourcemap generation using it, but really sourcemaps would still be generated for many files by pluginContainer.transform() anyway. So a proper name would be for example jsSourcemaps, but the question than boils down to this:
If everyone is happy generating sourcemaps for many-many files, why exclude *.js files? If someone is concerned with performance, probably a reasonable expectation could be to turn off sourcemaps all together or turn them on alltogether, if not. But the current behaviour does not serve either group: has performance impact for many files, but breaks debugging for *.js files... So if we were to add an option, it should also turn off generation in pluginContainer.transform() - that way it would be a performance tweak, for those who don't want sourcemaps. However I'd argue in that case that's a separate feature, while this is supposed to be a fix of the existing, but inconsistent behaviour.

*: Vite without this patch already generates full blown sourcemaps:

• all files processed by vite:esbuild e.g. *.ts & *.vue containing typescript
• all files that has a sourcemap to start with, e.g. packages/vite/dist/client/env.mjs
• all files processed by vite:json, so basically any *.json files.
• all files processed by vite:css, so basically any *.css, *.less, *.styl, *.sass etc. files.

So long story short, we are talking about an overhead for *.js files that is already accepted for many other file types.

Moreover the changes in this PR also attempt to further reduce the sourcemapping overhead by only generating the mappings part, if the file contents are changed (rewritten imports). If there are no changes, it only injects the sources & sourcesContent, and mappings are not generated so in that sense it still attempts to be as conservative with resource usage as possible.

Right now I'm using my patched Vite for development and debugging works great. I see no visible performance impact whatsoever and it is such an ease to be able to properly debug in the IDE for many cases that had issues previously.

Please let me know your thoughts, because I don't find the performance argument to be significant based on the above.

[sapphi-red]

I just wanted to explore a way not to add an additional perf cost. I agree the implication won't be significant.
I think we don't need an option but I'll put in the team board to discuss it in the team meeting.

Since Vite rewrites imports, and imports may be formatted in source code to spread multiple lines a proper SourceMap will still be needed even in VSCode in those cases, because as soon as the line numbers shift (as Vite rewrites the imports) a simple file path mapping won't be enough for proper debugging.

BTW Vite avoids the line numbers to be shifted when rewriting imports. So this won't happen. (If it does that's a bug.)

[BenceSzalai]

but I'll put in the team board to discuss it in the team meeting.

Thank you! I hope it'll go through! 🤞

BTW Vite avoids the line numbers to be shifted when rewriting imports. So this won't happen. (If it does that's a bug.)

That's great news! Maybe I saw it happening some time with v2, but maybe I was mistaken. Anyway, happy to hear that!

[sapphi-red]

That's great news! Maybe I saw it happening some time with v2, but maybe I was mistaken. Anyway, happy to hear that!

Ah, yeah, in v2, there was some and fixed in v3.

[sapphi-red]

In the last meeting, we decided that we can go without an option if there aren't perf implications.

I tested this out with some projects and didn't find differences in that point. So I think it's fine.

[Enzojz]

Just installed 4.0-alpha-5 for preview, in vscode it stops at breakpoint at correct file, but when I remove the breakpoint they still stop until I close the attached browser.

[BenceSzalai]

@Enzojz Answered here: #7767 (comment)

[BenceSzalai]

By the way, I can see it is merged in 4.0.0-alpha.6. Has release cycle for v3.x ended, or would this still land in v3 as well?

[Enzojz]

@Enzojz Answered here: #7767 (comment)

Has just replied there.