fix: breakpoints in JS not working #13514

sapphi-red · 2023-06-14T11:16:39Z

Description

I once struggled making this work without adding a big overhead caused by generating sourcemaps (#9476).
This time I came up with a solution using //# sourceURL.

I tested this with #13503 on Chrome and Firefox (Windows) and it worked for me.

This doesn't work with VSCode's debugger. 😢

Additional context

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 PR Title 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 #123).
Ideally, include relevant tests that fail without this PR but pass with it.

stackblitz · 2023-06-14T11:16:42Z

Run & review this pull request in StackBlitz Codeflow.

patak-dev · 2023-06-14T12:39:59Z

Nice! If this isn't a regression for VS Code, I think we can merge it and get some mileage from users in the 4.4 beta.

One thing I see is that the source is being applied to some files where it doesn't help. I imagine it isn't hurting though, but we could try to apply it only if the sourceURL points to a real file? Or maybe this would later be useful with virtual modules too?
From https://vite.new/vue:

//# sourceURL=/src/App.vue?vue&type=style&index=0&scoped=7a7a37b1&lang.css
//# sourceURL=/src/style.css

In the second case, it points to a real file but adding a breakpoint isn't useful. Still, I don't think it would hurt having the comment there. About 1., should it point to /src/App.vue?

bmeurer · 2023-06-15T06:16:36Z

I know this sounds obvious and like a great solution and @jaro-sevcik and I discussed this as well yesterday and whether we should recommend this as a fix. However, this isn't going to work as you expect it to work (with Chrome), because //# sourceURL (unlike //# sourceMappingURL) is handled by V8 (the back-end) while source maps are handled purely by the DevTools front-end. The implication is that if you have multiple scripts with a clashing //# sourceURL (intentionally clashing in the case of HMR), a breakpoint will be set in all of these scripts by V8, which is not what you want. Ideally only the last script would be around with HMR, but often you have state that keeps the previous versions of the script alive as well, and you'll stop in random places, which leads to a very confusing developer experience.

While this is more of an implementation detail of V8/Chrome, and I think we could also change it over time (although the specified behavior for //# sourceURL is even more vague than for source maps), I'd strongly encourage to go with identity source maps instead for now. They are extremely cheap to generate (you're merely adding couple milliseconds, which in the grand scheme of things doesn't matter) and will provide a consistent experience to developers.

sapphi-red · 2023-06-16T05:50:57Z

If this isn't a regression for VS Code,

It's a partial regression. Without this PR, the breakpoints work if that file is not changed. But with this PR, the breakpoints don't work even if that file is not changed.

However, this isn't going to work as you expect it to work (with Chrome),

I'd strongly encourage to go with identity source maps instead for now.

I see. To generate an identity source map, Vite will have to tokenize the content of the file and generate the mappings field. Or skip the tokenization and generate the mapping field with the every character in the file. Is there a better way than this?

bmeurer · 2023-06-16T05:57:22Z

To generate an identity source map, Vite will have to tokenize the content of the file and generate the mappings field. Or skip the tokenization and generate the mapping field with the every character in the file. Is there a better way than this?

I'd use a naive approach and just generate a mapping for every whitespace/word boundary (poor mans tokenization).

sapphi-red · 2023-08-12T08:07:35Z

I updated the PR to inject //# sourceMappingURL=data:* to every file unless { mappings: '' } is returned. I used hires: 'boundary' to generate the identity sourcemap as suggested.

I tested this with Chrome and Firefox and VSCode debugger on Windows. It worked fine.

sapphi-red · 2023-08-12T08:08:47Z

packages/vite/rollup.config.ts

-    plugins: [...createNodePlugins(false, false, false), bundleSizeLimit(120)],
+    plugins: [...createNodePlugins(false, false, false), bundleSizeLimit(150)],


It's now 149kB because magic-string is now bundled in the CJS build.

sapphi-red · 2023-08-12T08:10:12Z

packages/vite/src/node/server/send.ts

+    if (type === 'js' && (!map || map.mappings !== '')) {
+      const urlWithoutTimestamp = removeTimestampQuery(req.url!)
+      const ms = new MagicString(content.toString())
+      content = getCodeWithSourcemap(
+        type,
+        content.toString(),
+        ms.generateMap({ source: urlWithoutTimestamp, hires: 'boundary' }),
+      )
+    }


This part is the main part of the change. Other changes are to pass { mappings: '' } as-is to this part.

sapphi-red · 2023-08-12T08:13:42Z

packages/vite/src/node/server/transformRequest.ts

@@ -31,7 +31,7 @@ const debugCache = createDebugger('vite:cache')

 export interface TransformResult {
  code: string
-  map: SourceMap | null
+  map: SourceMap | { mappings: '' } | null


This change is a breaking change.
But this type was not correctly declared and actually transformRequest could return { mappings: '' } if a load hook returned a map with that and no transform hook transformed that request.

sapphi-red · 2023-08-12T08:15:52Z

packages/vite/src/node/server/pluginContainer.ts

+          // { mappings: '' }
+          if ((m as SourceMap).mappings === '') {
+            combinedMap = { mappings: '' }
+            break
+          }
          // empty, nullified source map
-          combinedMap = this.combinedMap = null
-          this.sourcemapChain.length = 0
+          combinedMap = null
          break


This part was not correct. { mappings: '' } is different from null. So it shouldn't be changed to null.
For example, if this.sourcemapChain is [{ mappings: '' }, validSourcemap], the combined result should be { mappings: '' } instead of validSourcemap.

sapphi-red · 2023-08-12T08:29:06Z

/ecosystem-ci run

vite-ecosystem-ci · 2023-08-12T08:29:30Z

📝 Ran ecosystem CI: Open

suite	result
analogjs	✅ success
astro	✅ success
histoire	✅ success
ladle	✅ success
laravel	✅ success
marko	✅ success
nuxt	❌ failure
nx	✅ success
previewjs	✅ success
qwik	✅ success
rakkas	✅ success
sveltekit	✅ success
unocss	❌ failure
vite-plugin-pwa	✅ success
vite-plugin-ssr	✅ success
vite-plugin-react	✅ success
vite-plugin-react-pages	❌ failure
vite-plugin-react-swc	✅ success
vite-plugin-svelte	❌ failure
vite-plugin-vue	✅ success
vite-setup-catalogue	✅ success
vitepress	✅ success
vitest	❌ failure

sapphi-red · 2023-08-12T08:58:52Z

nuxt, unocss, vite-plugin-react-pages are failing on main too.
vite-plugin-svelte is failing because of the snapshot mismatch that is expected.
vitest is failing with the type mismatch (#13514 (comment)).

patak-dev

I think it is a good idea to merge this one soon so other folks will test it during the Vite 5 beta. @bluwy go for it if you are good with these changes.

bluwy · 2023-08-21T13:21:55Z

Just reviewed this. LGTM 👍 Feel free to merge this once the conflicts are resolved @sapphi-red. Looks like it's caused by #14061, so the bundle limit numbers might change.

sapphi-red added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jun 14, 2023

sapphi-red mentioned this pull request Jul 28, 2023

perf: use magic-string hires boundary for sourcemaps #13971

Merged

9 tasks

sapphi-red added 5 commits August 12, 2023 01:54

fix: breakpoints in JS not working

60f1552

wip: use sourcemap

95ffb78

wip: use sourcemap

06c3d3b

wip: support { mappings: '' }

1690dc3

wip: update test

cc70dd4

sapphi-red added the feat: hmr label Aug 12, 2023

sapphi-red force-pushed the fix/js-breakpoints-not-working branch from 3e6073c to cc70dd4 Compare August 12, 2023 08:01

sapphi-red marked this pull request as ready for review August 12, 2023 08:01

sapphi-red commented Aug 12, 2023

View reviewed changes

patak-dev previously approved these changes Aug 14, 2023

View reviewed changes

chore: merge main

9d27b56

sapphi-red dismissed patak-dev’s stale review via 9d27b56 August 22, 2023 12:15

sapphi-red merged commit 0156bd2 into vitejs:main Aug 22, 2023

sapphi-red deleted the fix/js-breakpoints-not-working branch August 22, 2023 12:31

This was referenced Aug 29, 2023

Breakpoints lost in JS files with HRM #13503

Closed

Missing/broken sourcemaps for JS modules w/ imports when used with Vue #7767

Closed

fix: use relative path for sources field #14247

Merged

Cannot change SourceMaps path if esbuild is not used #8081

Closed

github-actions bot mentioned this pull request Apr 16, 2024

vite/5.2.9 package update wolfi-dev/os#16881

Merged

ArnaudBarre mentioned this pull request May 20, 2024

Breakpoint on initial render not being called on rerender vitejs/vite-plugin-react#179

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: breakpoints in JS not working #13514

fix: breakpoints in JS not working #13514

sapphi-red commented Jun 14, 2023

stackblitz bot commented Jun 14, 2023

patak-dev commented Jun 14, 2023

bmeurer commented Jun 15, 2023

sapphi-red commented Jun 16, 2023

bmeurer commented Jun 16, 2023

sapphi-red commented Aug 12, 2023

sapphi-red Aug 12, 2023

sapphi-red Aug 12, 2023

sapphi-red Aug 12, 2023

sapphi-red Aug 12, 2023

sapphi-red commented Aug 12, 2023

vite-ecosystem-ci bot commented Aug 12, 2023 •

edited

Loading

sapphi-red commented Aug 12, 2023

patak-dev left a comment

bluwy commented Aug 21, 2023

		plugins: [...createNodePlugins(false, false, false), bundleSizeLimit(120)],
		plugins: [...createNodePlugins(false, false, false), bundleSizeLimit(150)],

fix: breakpoints in JS not working #13514

fix: breakpoints in JS not working #13514

Conversation

sapphi-red commented Jun 14, 2023

Description

Additional context

What is the purpose of this pull request?

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

stackblitz bot commented Jun 14, 2023

patak-dev commented Jun 14, 2023

bmeurer commented Jun 15, 2023

sapphi-red commented Jun 16, 2023

bmeurer commented Jun 16, 2023

sapphi-red commented Aug 12, 2023

sapphi-red Aug 12, 2023

Choose a reason for hiding this comment

sapphi-red Aug 12, 2023

Choose a reason for hiding this comment

sapphi-red Aug 12, 2023

Choose a reason for hiding this comment

sapphi-red Aug 12, 2023

Choose a reason for hiding this comment

sapphi-red commented Aug 12, 2023

vite-ecosystem-ci bot commented Aug 12, 2023 • edited Loading

sapphi-red commented Aug 12, 2023

patak-dev left a comment

Choose a reason for hiding this comment

bluwy commented Aug 21, 2023

vite-ecosystem-ci bot commented Aug 12, 2023 •

edited

Loading