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: use relative path for sources field #14247

Merged

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Aug 31, 2023

Description

Follow up to #13514

#7767 (comment)
@BenceSzalai Thanks for the detailed comment! Would you try this PR if it fixes the bug you described?

fixes #7767

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.

@sapphi-red sapphi-red added p3-minor-bug An edge case that only affects very specific usage (priority) feat: sourcemap Sourcemap support labels Aug 31, 2023
@stackblitz
Copy link

stackblitz bot commented Aug 31, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@BenceSzalai
Copy link
Contributor

Would you try this PR if it fixes the bug you described?

Should I do a build or will there be another beta any soon? Or do you have a nightly perhaps? Or is it possible to grab the built asset from the CI runs maybe?

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Have a question below, but using path.basename makes sense to me.

@@ -69,7 +70,11 @@ export function send(
content = getCodeWithSourcemap(
type,
content.toString(),
ms.generateMap({ source: urlWithoutTimestamp, hires: 'boundary' }),
ms.generateMap({
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated, but a question: Since #13514, if the JS code has no sourcemap (map is falsy), we're still creating a fallback sourcemap here.

If there's no sourcemap, shouldn't we don't need to do anything?

Copy link
Member Author

@sapphi-red sapphi-red Sep 4, 2023

Choose a reason for hiding this comment

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

Unfortunately, to make dev tools happy, we'll always have to generate a sourcemap (#13514 (comment)).
Just in case, we are skipping sourcemap generation for { "mappings": "" } because it indicates generating a sourcemap is meaningless.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, thats feels like unnecessary overhead if the user never debugs a file, but good to know that it's needed for cases like this.

@bluwy
Copy link
Member

bluwy commented Sep 4, 2023

@BenceSzalai at the moment you'd need to clone Vite locally and build it to test it out

@sapphi-red
Copy link
Member Author

@BenceSzalai Sorry I forgot to reply, as bluwy said you'd need to build. Our CI don't upload the built files.

@BenceSzalai
Copy link
Contributor

Hi! Didn't really have the spare time to grab the repo and run a new build, but since v5.0.0-beta.1 has just dropped I've tested again, and I can confirm that - at least in my usecases - JS files are now properly mapped to the original source and the IDE is picking up the breakpoints properly!

Great news, thank you everyone!

@sapphi-red
Copy link
Member Author

@BenceSzalai Thanks for testing it out! But that's interesting, this PR isn't included in beta.1.
Here's the built tgz of this PR: vite-5.0.0-beta.0.tar.gz
It'd be nice if you can try this one out.

@BenceSzalai
Copy link
Contributor

BenceSzalai commented Sep 10, 2023

Okay. So first things first, I was sloppy testing this out, because I should have created multiple files with similar path and name to see if the IDE finds the right one, but did not, so it found one out of one, but that was working even with the first version of the fix, my bad.

So before moving on I've made two files with the same path and name in two different folders to make sure the issue is still there, and sure it is with 5.0.0-beta.1 too. The IDE fails to find the right one and just show a temporary file as provided by the browser.

As per the build you have provided, it looks like to me it behaves slightly better.

Test case1:
Make two files with the same filename in two different folders, put a breakpoint in one of them.

  • With the previous patch the IDE cannot decide which is the right file, so it shows a temporary file with the contents as seen by the browser.
  • With the new patch the IDE correctly identifies the right source file and shows the code paused at the breakpoint as set.

Test case 2:
Delete one of the files, so now there is only one file with the given name, so it can be identified unambiguously.

  • Both with the old and the new patch the IDE shows the execution paused in the right file.

My theory is that the IDE understands the content of sources relative to the path of the file executed in the browser, so when the sources is only a filename, it works well, but with path+filename the resolution fails.

As a sidenote: There is a possibility in IntelliJ to explicitly map folders of the local filesystem to the URLs server by vite (dev mode). If I put that in place, debugging is perfect with both the old and the new patch. This was not the case before the first patch, because we need two things for debugging:
1.) proper mapping of files as seen being served under a given URL by Vite dev to the files on the filesystem
2.) parts of the sourcemaps that provide the mapping between the transformed line numbers and the source file line numbers

1.) was possible by configuring IntelliJ options, even without changes to Vite, and I remember someone mentioned the last time we tried to fix this that such mapping is also possible in VSCode, although I've never tried to figure that one out.

2.) is however we need Vite to provide the sourcemaps.

Now 2.) is fulfilled by both this version and the previous patch.

What the new patch gives on top: The matching of the right source file succeeds in more edge cases without explicitly mapping the URLs to the filesystem (same filename in different folders).

Regarding adding sourcesContent: It is not proved by my experiments that IntelliJ IDE cares about this contrary to what I assumed before. So it is not important from the perspective of Browser+IDE debugging. But I'd say it is still useful if someone does debugging in the browser, which otherwise does not have access to the original file. But as said in the other thread the only difference is that without sourcesContent a strange //# sourceMappingURL=... line appears at the end of the mapped files in the Browser. Not a big deal, but also adding sourcesContent is not a big overhead, so really adding and not adding are both good options.

@sapphi-red
Copy link
Member Author

@BenceSzalai Thank you for testing this out!

@sapphi-red sapphi-red merged commit a995907 into vitejs:main Sep 10, 2023
1 check passed
Gaubee pushed a commit to Gaubee/vite that referenced this pull request Sep 13, 2023
@Soarex16
Copy link

Soarex16 commented Nov 23, 2023

Hello, @sapphi-red!
It looks like you generate source maps for .js files after path & alias resolution, so MagicString can't recognize changes you made during import transformation.
Let's suppose that I have file %PROJECT_ROOT%/src/logger.js

import {count, increment} from "./log-counter.js";

export function log(value) {
    increment();
    console.log(`[${count}] ${value}`)
}

If I start vite, attach with debugger (Chrome), then I'll get the following source:

import {count, increment} from "/src/log-counter.js";

export function log(value) {
    increment();
    console.log(`[${count}] ${value}`)
}

For comparison, imports are preserved in TypeScript:
Same example with logger.
Source:

import {count, increment} from "./log-counter.ts";

export function log(value: any) {
    increment();
    console.log(`[${count}] ${value}`)
}

Source received by debugger:

import {count, increment} from "./log-counter.ts";

export function log(value: any) {
    increment();
    console.log(`[${count}] ${value}`)
}

@sapphi-red sapphi-red deleted the fix/use-relative-path-for-sources-field branch November 24, 2023 08:30
@sapphi-red
Copy link
Member Author

@Soarex16 Thanks for the report. #15135 should fix that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: sourcemap Sourcemap support p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing/broken sourcemaps for JS modules w/ imports when used with Vue
4 participants