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

Revert "Use file URL for source map paths (#1771)" #1821

Merged
merged 1 commit into from
Jul 2, 2022

Conversation

cspotcode
Copy link
Collaborator

@cspotcode cspotcode commented Jul 2, 2022

Revert #1771

Fixes #1797
Fixes #1790

@PaperStrike if we choose to implement #1769 in the future, it'll need a bit more work to:

a) prove that we are fixing a real-world use-case, not merely an academic desire to comply with a spec
b) prove that debugging scenarios continue to work. Will likely require automated testing of:

  • VSCode debugging
  • code coverage via nyc
  • code coverage via c8 which, in turn, uses node's native coverage mechanism

@cspotcode cspotcode added this to the 10.8.2 milestone Jul 2, 2022
@codecov
Copy link

codecov bot commented Jul 2, 2022

Codecov Report

Merging #1821 (861f3e9) into main (bf13086) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
src/index.ts 79.95% <100.00%> (-0.09%) ⬇️

@cspotcode cspotcode marked this pull request as ready for review July 2, 2022 16:24
@PaperStrike
Copy link
Contributor

PaperStrike commented Jul 2, 2022

I have no time for issuing an academic desire that has no real world use case. I forgot to mention that it's found when using c8 with --exclude-after-remap on Windows. But recently I am really really busy, I am not (not until about July 9) going to prepare a repro or a test case or things other than commenting, sorry about that.

@cspotcode
Copy link
Collaborator Author

Time constraints are understandable; I've been very busy myself, so I understand where you're coming from.

I think there may be some confusion about my comments.

This change broke nyc coverage reports and debugging scenarios. Given the evidence available to me, it looks like this change did more harm than good. So reverting is the most appropriate action. We can always discuss further in the future if/when you or anyone else has the time to investigate.

@cspotcode cspotcode merged commit 8aedaed into main Jul 2, 2022
@cspotcode cspotcode deleted the revert-sourcemap-file-uri-change branch July 2, 2022 16:50
@PaperStrike
Copy link
Contributor

I totally understand that. Maybe we can try the basename solution in the future.

I remember the problem was like, c8 totally relies on test-exclude when --exclude-after-remap is enabled, so it met bcoe/c8#183 and istanbuljs/test-exclude#43 when combining with ts-node and Windows. The two issues aren't resolved for some reason, so I filled #1769 here.

@cspotcode
Copy link
Collaborator Author

Yeah I had no luck getting in touch with anyone from nyc. I don't think nyc has any meaningful maintainership. Which is unfortunate, nyc clearly needs a patch to support file URIs.

The debugger breakpoints issue, I haven't had time to investigate. I created a ticket #1822 for setting up some sort of automated testing for these things. I have no idea how I'll do it, but I think it will be important. And then perhaps those tests will help us validate the basename solution.

I want to thank you again for your contribution, even though we had to revert it.

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants