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: ignore ENOENT in injectSourcesContent #2904

Merged
merged 5 commits into from Jul 21, 2021

Conversation

aleclarson
Copy link
Member

@aleclarson aleclarson commented Apr 7, 2021

Description

This is important for virtual modules and malformed source maps, both of which can throw ENOENT errors when injecting sourcesContent into source maps.

Solution

Expect sourcemaps for virtual modules to either contain a pre-populated sourcesContent array or have a sources array of absolute paths only. Otherwise, a warning will be printed (at most one time).

Expect sourcemaps for normal modules to either contain a pre-populated sourcesContent array or have a sources array that never points to non-existent files. Relative source paths are allowed, and they're resolved with sourceRoot (if defined) or the parent directory of the module. When a non-existent source is found, a warning will printed (at most once per sourcemap for the lifetime of the Vite server).


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

patak-dev
patak-dev previously approved these changes Apr 8, 2021
@patak-dev
Copy link
Member

Would it be possible to differentiate between malformed source maps and virtual modules so we can emit a warning when the maps are malformed instead of a silent ignore? Also, do you think it is possible to add a test case?

@Shinigami92
Copy link
Member

Is this p2-nice-to-have or p3-minor-bug?

@patak-dev patak-dev added the p3-minor-bug An edge case that only affects very specific usage (priority) label Apr 8, 2021
@aleclarson
Copy link
Member Author

Would it be possible to differentiate between malformed source maps and virtual modules so we can emit a warning when the maps are malformed instead of a silent ignore?

Maybe, should we bring this up with Evan?

@dimfeld
Copy link
Contributor

dimfeld commented Jul 19, 2021

Am I correct that with the changes to this PR it now throws errors once again in the "malformed sourcemap" case, as it currently does? I think it would be very helpful if malformed source maps in dependencies, whether from bad paths or just failure to package the source with the NPM package, didn't completely kill the build.

This is an especially sensitive case IMO because it mostly comes up with dependencies where there's a much longer turnaround to get the source maps fixed.

@aleclarson
Copy link
Member Author

@dimfeld That's a good point. It's not a fatal error, so we shouldn't kill the build. 👍

This is important for virtual modules and malformed source maps, both of which can throw ENOENT errors when injecting `sourcesContent`.
Also, when the `file` is a virtual module, assume source paths are already absolute.
@aleclarson
Copy link
Member Author

aleclarson commented Jul 21, 2021

@patak-js @antfu This PR has been reworked and rebased.
Please read the "Solution" section in the OP, and review the code. Thx

@patak-dev patak-dev merged commit 0693d03 into vitejs:main Jul 21, 2021
@HerringtonDarkholme
Copy link
Contributor

Hi @patak-js, I wonder if this PR could be released to a new version of Vite soon? Thanks!

@patak-dev
Copy link
Member

We are doing one release per week, it will be out next tuesday

@wmzy
Copy link
Contributor

wmzy commented Jul 22, 2021

Hi @aleclarson and @patak-js ,
I also encountered this bug.
I debug and found the transformResult.map.sourcesContent will be excluded always:

const transformResult = await pluginContainer.transform(code, id, map, ssr)
if (
transformResult == null ||
(isObject(transformResult) && transformResult.code == null)
) {
// no transform applied, keep code as-is
isDebug &&
debugTransform(
timeFrom(transformStart) + chalk.dim(` [skipped] ${prettyUrl}`)
)
} else {
isDebug && debugTransform(`${timeFrom(transformStart)} ${prettyUrl}`)
code = transformResult.code!
map = transformResult.map
}
if (map && mod.file) {
map = (typeof map === 'string' ? JSON.parse(map) : map) as SourceMap
if (map.mappings && !map.sourcesContent) {
await injectSourcesContent(map, mod.file, logger)
}
}

The transform will call
export function combineSourcemaps(
to combine sourcemap which will call remapping with excludeContent option as true.

@patak-dev
Copy link
Member

@wmzy do you see an issue then with the way this PR is implemented? Could you create a bug report if that is the case?

@wmzy
Copy link
Contributor

wmzy commented Jul 23, 2021

@patak-js this PR fixed the bug in injectSourcesContent .
But in my case, the injectSourcesContent shoud not been call if the pluginContainer.transform has not lost the sourcesContent.

I will try a PR to optimize this. It's hard to explain with my poor English.

@wmzy wmzy mentioned this pull request Jul 23, 2021
9 tasks
@wmzy
Copy link
Contributor

wmzy commented Jul 23, 2021

@patak-js The PR has been created, could you review it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Vite fails to build when source maps link to non existing file with error: "ENOENT: no such file or directory"
7 participants