-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
Broken V8 source map cache file URL parsing on Windows #143
Comments
@clemyan I like your suggestion of trying to use |
Well the bug only started happening since some where in the v15 line of node (when the file URL format changed), and I saw you made some changes in node 15.11.0 regarding source maps, so I have tested against 15.10.0, but it is still affected. In other words:
Regarding I am seeing a number of issues over the years both here and in
If you are interested, I can open a separate mega-issue and see if we can work out a solution. |
Today we support a source map that may map to multiple source files, but you're saying that there may be cases where one source file has multiple source maps?
I agree that we should make any changes necessary to get us inline with the spec.
Let's bump to the engines field to I'm not quite ready to drop Node 10 support in
I would certainly be interested in any additional fixes you are able to submit 👍 I would love to break the work into small chunks if possible. |
Yes. If you need a concrete case:
In this case, both Crude idea:
I will try to whip up a PoC over the week.
I will see what I can do. And I forgot to mention this before. So I said tests don't run on Windows, but I was wrong. I messed up when swapping around node versions to test this issue. Turns out I was just running into tapjs/tapjs#746. So that wasn't a Windows/Linux thing. In terms of results, looks like the test are failing due to Windows-related issues: (Node 14.16.1)
|
@clemyan is the initial issue addressed now, with the patch we landed on c8? I'm wondering if we can close this issue, and open issues in the future for any additional work you'd like to take on. |
@bcoe For my use case, yes, since I run my test suite via ts-node directly. But the c8 test cases for precompiled TypeScript and UglifyJS are still failing (on Windows Node 16) due to this issue. Since you are open to bumping up to I'll open a separate issue for the other stuff. |
Somewhere along the node v15 line, V8 coverage data output by node changed format on Windows. Specifically, file URLs within the
source-map-cache
changed fromto
(Note the extra forward slash in front)
I haven't bisected further but v15.0.0 is using backslashes and v15.14.0 is using forward slashes.
This causes
_resolveSource
to return incorrect file path on Windows (e.g./C:/some/path/main.js
), in turn causingload
to reject when trying to read the wrong path from fs.The easy solution would be to check
process.platform
and strip any leading forward slash, but I was thinking whether there is a more robust solution. A few ideas/notes:v8-to-istanbul
supports down to node 10.10.0, we cannot rely onfileURLToPath
which was added in 10.12.0. Or we can just copy node's implementation?webpack://
protocol: looks like URLs inside webpack sourcemaps are fully configurable? But the default format look pretty easily parsable usingnew URL
new URL
? Seem doable if we ignore UNC paths (which I don't think currently works anyway?)The biggest challenge currently is the tests doesn't want to run at all on Windows machines (Well, on my Windows machine, but running the tests inside a Linux docker container on the same machine works so I am pretty sure it is a Windows thing), so I can't easily experiment.
The text was updated successfully, but these errors were encountered: