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.

Sign up for GitHub

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: avoid null sourcePath in server.sourcemapIgnoreList #12251

Merged
merged 1 commit into from
Mar 1, 2023
Merged

fix: avoid null sourcePath in server.sourcemapIgnoreList #12251

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions packages/vite/src/node/server/transformRequest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ async function loadAndTransform(
++sourcesIndex
) {
const sourcePath = map.sources[sourcesIndex]
if (!sourcePath) continue

const sourcemapPath = `${mod.file}.map`
const ignoreList = config.server.sourcemapIgnoreList(
Expand All @@ -298,11 +299,7 @@ async function loadAndTransform(
// Rewrite sources to relative paths to give debuggers the chance
// to resolve and display them in a meaningful way (rather than
// with absolute paths).
if (
sourcePath &&
path.isAbsolute(sourcePath) &&
path.isAbsolute(mod.file)
) {
if (path.isAbsolute(sourcePath) && path.isAbsolute(mod.file)) {
map.sources[sourcesIndex] = path.relative(
Copy link
Member Author

Choose a reason for hiding this comment

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

@danielroe was it discussed if we should call sourcemapIgnoreList using the sourcePath after 303? I don't know what is best here, but maybe it isn't a bad idea to get the path that will end up showing in the dev tools?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @bmeurer and I felt it would be better to call it with an absolute path: #12174 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the reference, I missed that one. @bmeurer what is the reason you consider safer to go with absolute paths? My first reaction while reading those messages was of suggesting to align as close as possible with rollup (we still have time to change this if we think it is better).

Copy link
Contributor

Choose a reason for hiding this comment

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

rollup passes project relative paths. I guess that would be fine here as well, but I'm not sure how to reconstruct the project relative paths here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked with a fresh create-vite Vue Typescript app, and the paths I currently get are:

sourcemapIgnoreList: (sourcePath, sourcemapPath) => boolean

> pnpm run build

sourcePath: ../../node_modules/.pnpm/@[email protected]/node_modules/@vue/shared/dist/shared.esm-bundler.js
sourcemapPath: /Users/patak/test/sourcemap-test/dist/assets/index-!~{001}~.js.map

sourcePath: ../../node_modules/.pnpm/@[email protected]/node_modules/@vue/reactivity/dist/reactivity.esm-bundler.js
sourcemapPath: /Users/patak/test/sourcemap-test/dist/assets/index-!~{001}~.js.map

sourcePath: ../../node_modules/.pnpm/@[email protected]/node_modules/@vue/runtime-core/dist/runtime-core.esm-bundler.js
sourcemapPath: /Users/patak/test/sourcemap-test/dist/assets/index-!~{001}~.js.map

sourcePath: ../../node_modules/.pnpm/@[email protected]/node_modules/@vue/runtime-dom/dist/runtime-dom.esm-bundler.js
sourcemapPath: /Users/patak/test/sourcemap-test/dist/assets/index-!~{001}~.js.map

sourcePath: ../../../../../../vite.svg
sourcemapPath: /Users/patak/test/sourcemap-test/dist/assets/index-!~{001}~.js.map

sourcePath: ../../src/assets/vue.svg
sourcemapPath: /Users/patak/test/sourcemap-test/dist/assets/index-!~{001}~.js.map

sourcePath: ../../src/components/HelloWorld.vue
sourcemapPath: /Users/patak/test/sourcemap-test/dist/assets/index-!~{001}~.js.map

sourcePath: ../../src/main.ts
sourcemapPath: /Users/patak/test/sourcemap-test/dist/assets/index-!~{001}~.js.map

So it looks like the sourcePath in rollup is relative to the sourcemapPath, no?

For dev we get

> pnpm run dev

sourcePath: overlay.ts
sourcemapPath: /Users/patak/vite-work/packages/vite/dist/client/client.mjs.map

sourcePath: client.ts
sourcemapPath: /Users/patak/vite-work/packages/vite/dist/client/client.mjs.map

sourcePath: /Users/patak/test/sourcemap-test/src/main.ts
sourcemapPath: /Users/patak/test/sourcemap-test/src/main.ts.map

sourcePath: /Users/patak/test/sourcemap-test/src/App.vue
sourcemapPath: /Users/patak/test/sourcemap-test/src/App.vue.map

sourcePath: env.ts
sourcemapPath: /Users/patak/vite-work/packages/vite/dist/client/env.mjs.map

sourcePath: /Users/patak/test/sourcemap-test/src/components/HelloWorld.vue
sourcemapPath: /Users/patak/test/sourcemap-test/src/components/HelloWorld.vue.map

The ones we care about in this discussion are the .vue files and the main.ts. So, if we want to align with rollup, I think sourcePath would end up being main.ts, App.vue, and HelloWorld.vue. So the file relative to .map path of sourcemapPath. This means that users should mainly be using sourcemapPath to do the condition instead of sourcePath during dev. The absolute path seems easier to work with as a user. Maybe the alignment isn't that important here. cc @bluwy @sapphi-red for a second opinion.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth aligning with Rollup here to avoid the confusion that it works in dev, and later not in builds. We could rename the first param as relativeSourcePath like Rollup so it's consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM.

path.dirname(mod.file),
sourcePath,
Expand Down