10.8.1 regression - URL polluting relative file path
Jason3S opened this issue · 14 comments
When upgrading from 10.8.0 to 10.8.1, coverage reports started breaking.
10.8.0
10.8.1
I think this is related to Use file URL for source map paths by PaperStrike · Pull Request #1771 · TypeStrong/ts-node
Search Terms
Expected Behavior
That generating coverage reports still work.
Actual Behavior
Coverage reports are broken because they contain an absolute URL.
Steps to reproduce the problem
GitHub Repo: Jason3S/rx-stream
npm i
npm run coverage
cat coverage/lcov.infonpm i -SD ts-node@10.8.1
npm run coverage
cat coverage/lcov.infoMinimal reproduction
Specifications
- ts-node version:
- node version:
- TypeScript version:
- tsconfig.json, if you're using one:
{}
- package.json:
{}
- Operating system and version:
- If Windows, are you using WSL or WSL2?:
To confirm: file URIs in sourcemaps should be acceptable, right? I believe they are a part of the spec, and in fact, using native paths might be less spec compliant.
Is this a case where ts-node is doing something wrong, or is it that nyc is failing to understand file URIs?
I don't know the answer but I think we'll need to figure that out before choosing a course of action.
Same issue on our end. Thanks for reporting @Jason3S
Additionally, can someone create a minimal reproducible example? Will help to get this resolved quicker.
Please check with https://github.com/istanbuljs/istanbuljs to see if they support file URIs in sourcemaps. This may be a bug on their end. Regardless of what they say, asking will help get this resolved sooner.
Specifically, I think the relevant library is istanbul-lib-source-maps: https://github.com/istanbuljs/istanbuljs/tree/master/packages/istanbul-lib-source-maps
What real-world-problem was PR #1771 solving? I understand that it was putting d:/absolute/path/to/file into the source map. But Source Map Cache is still experimental:
Since this is a breaking change, putting it behind a command-line switch would be a way to move forwards without breaking legacy libraries like nyc and istanbul.
The spec is a bit vague when it comes to URLs: https://sourcemaps.info/spec.html
Only the url field is clearly a URL.
If the sources are not absolute URLs after prepending of the “sourceRoot”, the sources are resolved relative to the SourceMap (like resolving script src in a html document).
So, I can understand if the Istanbul implementation doesn't handle absolute URLs in the map. Reading it, you could infer that sourceRoot could be file:// and sources are [ "/full/path/to/file.ts" ].
What real-world-problem was PR #1771 solving?
It was solving this bug: #1769
But Source Map Cache is still experimental:
We don't use that at all; we have our own source-mapping implementation. Node's source-map stuff has bugs, limitations, and performance issues. Hopefully the situation will improve in the future, but it's not there yet. But just to be totally clear, we are not using that node feature.
Since this is a breaking change
The intention was for it to not be breaking. Either we're triggering a bug in nyc, or we have a bug ourselves. This depends on if the sourcemaps we're generating are fully spec-compliant. As reported in #1769, putting D:/foo/bar.ts into a sourcemap is actually buggy behavior since sourcemaps are spec-ed to use file URIs.
The pragmatic solution may very well be for us to make a change on our end either way. But I want to be clear that this is not a breaking change in semver terms.
So, I can understand if the Istanbul implementation doesn't handle absolute URLs in the map. Reading it, you could infer that sourceRoot could be file:// and sources are [ "/full/path/to/file.ts" ].
I think in our case, sources are file:///full/path/to/file.ts. Assuming I'm interpreting that spec correctly, this means that the sources are absolute URLs after prepending of the sourceRoot, so they should not be resolved relative to the SourceMap. (copying the language from the spec)
Have you raised this with the istanbul team to see what they say?
I do see what you mean about #1769 using that experimental node feature. Happy to keep discussing and work towards a fix; my response above was written pretty quickly. Will await your reply.
This issue seems related to an issue I just encountered with 10.8.1 (10.8.0 works fine):
when doing remote debugging with VSCode, breakpoints are unbound. VSCode still hits breakpoints, but it shows them being hit in the remote directory rather than local source code.
Thanks @CharlesSwiftConnect , can you share a few more details in a new ticket? I'll link it to this one; just helps having reproductions and subsequent investigation in their own tickets.
same problem +1
Not to pile on, but +1. This has broken our builds because nyc creates "file:" folders in the coverage output and actions/upload-artifact fails due to the colon in the folder name
It seems that the change is about to be rolled back: #1797 (comment)
Glad to know other people hit this. Just spent all morning trying to figure out how these file: folders started showing up. I can confirm this only arises in 10.8.1. I'd assumed it was an update to Jasmine or Istanbul.
Like @jdforsythe mentions, this breaks artifact uploads; Azure DevOps in our case.
e.g.
Start uploading file 'coverage/lcov-report/services/mssql/sequelize/file:/__w/2/s/src/services/mssql/sequelize/basic_mssql_group.ts.html' to server, chunk '1'.
Chunk '1' attempt '2' of file 'coverage/lcov-report/services/mssql/sequelize/file:/__w/2/s/src/services/mssql/sequelize/basic_mssql_group.ts.html' fail to send request to server. Error: Microsoft.VisualStudio.Services.WebApi.VssServiceResponseException: TF10123: The path 'coverage/lcov-report/services/mssql/sequelize/file:/__w/2/s/src/services/mssql/sequelize/basic_mssql_group.ts.html' contains the character ':'. Remove the ':' and try again.