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

10.8.1 regression - URL polluting relative file path #1790

Closed
Jason3S opened this issue Jun 7, 2022 · 14 comments · Fixed by #1821 or JhayceFrancis/nativescript-nfc#13 · May be fixed by GLOBAL-FINTECH/api#47, YoutacRandS-VA/starter-workflows#3 or FEWS-NET/inf-node-gitlab-2-github#4
Milestone

Comments

@Jason3S
Copy link

Jason3S commented Jun 7, 2022

When upgrading from 10.8.0 to 10.8.1, coverage reports started breaking.

10.8.0
image

10.8.1
image

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.info
npm i -SD [email protected]
npm run coverage
cat coverage/lcov.info

Minimal 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?:
@cspotcode
Copy link
Collaborator

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.

@mark-studer
Copy link

Same issue on our end. Thanks for reporting @Jason3S

@cspotcode
Copy link
Collaborator

cspotcode commented Jun 7, 2022

Additionally, can someone create a minimal reproducible example? Will help to get this resolved quicker.

https://en.wikipedia.org/wiki/Minimal_reproducible_example

@cspotcode
Copy link
Collaborator

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

@Jason3S
Copy link
Author

Jason3S commented Jun 7, 2022

@cspotcode,

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:
image

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" ].

@cspotcode
Copy link
Collaborator

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?

@cspotcode
Copy link
Collaborator

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.

@CharlesSwiftConnect
Copy link

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.

@cspotcode
Copy link
Collaborator

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.

@luckyyyyy
Copy link

same problem +1

@luckyyyyy
Copy link

luckyyyyy commented Jun 16, 2022

jan-molak added a commit to serenity-js/serenity-js that referenced this issue Jun 23, 2022
This avoids issue introduced in ts-node 10.8.1 (see TypeStrong/ts-node#1790) and manifesting itself
file URLs polluting relative file paths and preventing NYC from producing code coverage reports (see
istanbuljs/nyc#1473)
@jdforsythe
Copy link

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

@pfedan
Copy link

pfedan commented Jun 28, 2022

It seems that the change is about to be rolled back: #1797 (comment)

@rootd00d
Copy link

rootd00d commented Jun 30, 2022

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.

@cspotcode cspotcode added this to the 10.8.2 milestone Jul 2, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Jul 11, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [ts-node](https://typestrong.org/ts-node) ([source](https://github.com/TypeStrong/ts-node)) | devDependencies | patch | [`10.8.1` -> `10.8.2`](https://renovatebot.com/diffs/npm/ts-node/10.8.1/10.8.2) |

---

### Release Notes

<details>
<summary>TypeStrong/ts-node</summary>

### [`v10.8.2`](https://github.com/TypeStrong/ts-node/releases/tag/v10.8.2)

[Compare Source](TypeStrong/ts-node@v10.8.1...v10.8.2)

**Fixed**

-   Revert "Use file URL for source map paths" ([#&#8203;1821](TypeStrong/ts-node#1821)) [@&#8203;cspotcode](https://github.com/cspotcode)
    -   Fixes [#&#8203;1790](TypeStrong/ts-node#1790): ts-node 10.8.1 regression where `nyc` code coverage reports had incorrect paths
    -   Fixes [#&#8203;1797](TypeStrong/ts-node#1797): ts-node 10.8.1 regression where breakpoints did not hit in VSCode debugging
-   Allow JSON imports in node 16.15 and up ([#&#8203;1792](TypeStrong/ts-node#1792)) [@&#8203;queengooborg](https://github.com/queengooborg)
    -   JSON imports were already supported in v17.5 and up
    -   this change extends support to >=16.15.0,<17.0.0
    -   These version ranges match vanilla node's support for JSON imports

https://github.com/TypeStrong/ts-node/milestone/15?closed=1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <[email protected]>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1446
Reviewed-by: Epsilon_02 <[email protected]>
Co-authored-by: Calciumdibromid Bot <[email protected]>
Co-committed-by: Calciumdibromid Bot <[email protected]>
cidinene added a commit to cidinene/asw2223_0 that referenced this issue Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment