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

Switch to new @cspotcode/source-map-support, using trace-mapping #1729

Merged
merged 14 commits into from
May 1, 2022

Conversation

cspotcode
Copy link
Collaborator

@cspotcode cspotcode commented Apr 21, 2022

Getting test failures because the mapped stack filenames look like this in some tests: (note the /file:/ mid-way through the path)

file:///d/ts-node/tests/esm/file:/d/ts-node/tests/esm/throw error.ts

Full example:

Command failed: node  --loader ts-node/esm \\"throw error.ts\\"
    (node:2859) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time␊
    (Use `node --trace-warnings ...` to show where the warning was created)
    Error: this is a demo
        at Foo.bar (file:///d/Personal-dev/@TypeStrong/ts-node/trace-mapping/tests/esm/file:/d/Personal-dev/@TypeStrong/ts-node/trace-mapping/tests/esm/throw error.ts:100:17)
        at new Foo (file:///d/Personal-dev/@TypeStrong/ts-node/trace-mapping/tests/esm/file:/d/Personal-dev/@TypeStrong/ts-node/trace-mapping/tests/esm/throw error.ts:98:10)
        at file:///d/Personal-dev/@TypeStrong/ts-node/trace-mapping/tests/esm/file:/d/Personal-dev/@TypeStrong/ts-node/trace-mapping/tests/esm/throw error.ts:102:1
        at ModuleJob.run (node:internal/modules/esm/module_job:197:25)
        at async Promise.all (index 0)
        at async ESMLoader.import (node:internal/modules/esm/loader:337:24)
        at async loadESM (node:internal/process/esm_loader:88:5)
        at async handleMainPromise (node:internal/modules/run_main:61:12)

@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #1729 (faea240) into main (9e25557) will not change coverage.
The diff coverage is n/a.

@jridgewell
Copy link

jridgewell commented Apr 22, 2022

Hi @cspotcode, I just published @jridgewell/resolve-uri v3.0.6 which should resolve your file: issues. You should pick it up by updating node modules.

@cspotcode
Copy link
Collaborator Author

Interesting, my testing tracked the issue down to some faulty logic in source-map-support, so I'm curious what was changed on your end. Did you make it detect whether the input paths are posix paths, not file URI, and if so, also return posix paths instead of file URI?

@jridgewell
Copy link

I'm assuming you have a have two file:/d/ URLs in a sourcemap (either a sourceRoot and a mapUrl when constructing the TraceMap, or an absolute sources array and a sourceRoot, etc). TraceMaps resolvedSources uses resolve-uri to join those together, and I didn't properly handle semi-valid file: URLs (they're supposed to be file:///foo/bar/…, not file:/foo/bar/…) when joining.

@cspotcode
Copy link
Collaborator Author

cspotcode commented Apr 23, 2022

For posterity, here is the fix:
https://github.com/cspotcode/node-source-map-support/pull/38/files#diff-6c09ea90fab3390be21a951054452be687b7de7b791f14e10874f785dc095864R189-R207

In this particular case, the fixes in trace-mapping weren't causing this problem. The repaired logic linked above needed to accept a file URI as the url argument. It was, funny enough, expecting a path instead of a url. When running native ESM modules, node's stack traces use file: URLs instead of absolute paths.

EDIT: not entirely sure if this logic is still required, if trace-mapping guarantees that all sources are coming to us as absolute file URI.


Some additional info, for curious readers:

At the time of writing, ts-node provides sourcemaps with file and sources that are absolute paths but not file URIs. (e.g. "file": "/root/project/generated.js") The sourcemaps do not have a sourceRoot. I don't remember the reasons it was implemented this way.

ts-node/src/index.ts

Lines 1546 to 1555 in 9e25557

/**
* Update the source map contents for improved output.
*/
function updateSourceMap(sourceMapText: string, fileName: string) {
const sourceMap = JSON.parse(sourceMapText);
sourceMap.file = fileName;
sourceMap.sources = [fileName];
delete sourceMap.sourceRoot;
return JSON.stringify(sourceMap);
}

However, source-map-support needs to handle all sourcemaps, not just those that come from ts-node.

@jridgewell
Copy link

jridgewell commented Apr 23, 2022

not entirely sure if this logic is still required, if trace-mapping guarantees that all sources are coming to us as absolute file URI

trace-mapping should handle absolute file paths without issue, just make sure the POSIX and not Windows format.

Might I recommend you switch to using @jridgewell/resolve-uri to perform relative URL joining? It behaves similar to new URL(url, baseUrl), but supports absolute URLs, protocol-relative URLs, absolute POSIX paths, and relative POSIX paths.

@cspotcode cspotcode marked this pull request as ready for review May 1, 2022 20:50
@cspotcode cspotcode added this to the 10.8.0 milestone May 1, 2022
@cspotcode cspotcode merged commit 17aad13 into main May 1, 2022
@cspotcode cspotcode deleted the trace-mapping branch May 7, 2022 13:58
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request May 30, 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 | minor | [`10.7.0` -> `10.8.0`](https://renovatebot.com/diffs/npm/ts-node/10.7.0/10.8.0) |

---

### Release Notes

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

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

[Compare Source](TypeStrong/ts-node@v10.7.0...v10.8.0)

Questions about this release? Ask in the official discussion thread: [#&#8203;1767](TypeStrong/ts-node#1767)

**Added**

-   Added support for `module=NodeNext`, `module=Node16`, `.mts`, `.cts`, `.mjs`, and `.cjs` file extensions ([#&#8203;1414](TypeStrong/ts-node#1414), [#&#8203;1694](TypeStrong/ts-node#1694), [#&#8203;1744](TypeStrong/ts-node#1744), [#&#8203;1745](TypeStrong/ts-node#1745), [#&#8203;1727](TypeStrong/ts-node#1727), [#&#8203;1717](TypeStrong/ts-node#1717), [#&#8203;1753](TypeStrong/ts-node#1753), [#&#8203;1757](TypeStrong/ts-node#1757)) [@&#8203;cspotcode](https://github.com/cspotcode)
    -   For best results, enable `experimentalResolver` ([docs](https://typestrong.org/ts-node/docs/options#experimentalresolver))
    -   See TypeScript's official documentation: https://www.typescriptlang.org/docs/handbook/esm-node.html
    -   enables mixed-mode projects with both ESM and CommonJS
    -   enables all supported file extensions in TypeScript 4.7
    -   Obeys package.json "type"
-   Added ability to include file extensions in CommonJS imports ([#&#8203;1727](TypeStrong/ts-node#1727), [#&#8203;1753](TypeStrong/ts-node#1753)) [@&#8203;cspotcode](https://github.com/cspotcode)
    -   Enables consistency with ESM, where file extensions are often mandatory
-   Resolves from emitted to source file extensions ([#&#8203;1727](TypeStrong/ts-node#1727), [#&#8203;1753](TypeStrong/ts-node#1753)) [@&#8203;cspotcode](https://github.com/cspotcode)
    -   Must enable `experimentalResolver`, will be enabled by default in a future version ([docs](https://typestrong.org/ts-node/docs/options#experimentalresolver))
    -   Typechecker requires importing the *emitted* file extension; ts-node resolves correctly to the *source* file.  E.g. `import "./foo.js"` will execute `foo.ts` See also: [TypeScript issue #&#8203;37582](microsoft/TypeScript#37582)
    -   If typechecking is disabled, you can also use *source* file extensions.  E.g. `import "./foo.ts"`
-   Added `experimentalSpecifierResolution` ([#&#8203;1727](TypeStrong/ts-node#1727), [#&#8203;1753](TypeStrong/ts-node#1753)) [@&#8203;cspotcode](https://github.com/cspotcode)
    -   the same as Node's `--experimental-specifier-resolution` ([Node docs](https://nodejs.org/dist/latest-v18.x/docs/api/esm.html#customizing-esm-specifier-resolution-algorithm))
    -   can also be specified in `tsconfig.json` for convenience, to avoid the CLI flag
    -   allows omitting file extensions in ESM imports, plus a few other CommonJS-style conveniences
-   Adds `diagnostics` property to `TSError`, with array of TypeScript diagnostic objects from the compiler ([API docs](https://typestrong.org/ts-node/api/classes/TSError.html)) ([#&#8203;1705](TypeStrong/ts-node#1705), [#&#8203;1706](TypeStrong/ts-node#1706)) [@&#8203;paulbrimicombe](https://github.com/paulbrimicombe)

**Changed**

-   Renames option `experimentalResolverFeatures` to `experimentalResolver` ([docs](https://typestrong.org/ts-node/docs/options#experimentalresolver)) ([#&#8203;1727](TypeStrong/ts-node#1727)) [@&#8203;cspotcode](https://github.com/cspotcode)
-   Internal change to ESM loader for compatibility with forthcoming node versions: returns `shortCircuit: true` ([#&#8203;1714](TypeStrong/ts-node#1714), [#&#8203;1715](TypeStrong/ts-node#1715)) [@&#8203;cspotcode](https://github.com/cspotcode)
-   Performance: Optimize filesystem stat calls in ESM loader and new CommonJS resolver ([#&#8203;1758](TypeStrong/ts-node#1758), [#&#8203;1759](TypeStrong/ts-node#1759)) [@&#8203;cspotcode](https://github.com/cspotcode)
-   Performance, maintenance: Upgrade source-mapper dependency "[@&#8203;cspotcode/source-map-support](https://github.com/cspotcode/source-map-support)"
    -   Switches to "trace-mapping" for underlying source-map parsing ([#&#8203;1729](TypeStrong/ts-node#1729)) [@&#8203;cspotcode](https://github.com/cspotcode)

**Fixed**

-   Fixed bug where REPL `.type` command was not showing any type information when using TypeScript nightly builds ([#&#8203;1761](TypeStrong/ts-node#1761), [#&#8203;1762](TypeStrong/ts-node#1762)) [@&#8203;cspotcode](https://github.com/cspotcode)
-   Correctly suppress "Custom ESM Loaders" warning on newer node versions where the warning's prose changed ([#&#8203;1701](TypeStrong/ts-node#1701)) [@&#8203;cspotcode](https://github.com/cspotcode)
-   Fixed REPL bug where function signatures could not be entered across multiple lines ([#&#8203;1667](TypeStrong/ts-node#1667), [#&#8203;1677](TypeStrong/ts-node#1677)) [@&#8203;d9k](https://github.com/d9k)
-   REPL treats unparenthesized object literals as objects, instead of as block scopes ([#&#8203;1697](TypeStrong/ts-node#1697), [#&#8203;1699](TypeStrong/ts-node#1699)) [@&#8203;jhmaster2000](https://github.com/jhmaster2000)
-   Fixed bug where `preferTsExts` combined with third-party transpiler hooks could disrupt `nyc` code coverage ([#&#8203;1755](TypeStrong/ts-node#1755)) [@&#8203;cspotcode](https://github.com/cspotcode)
-   Fixed bug where `file://` URLs in stack traces did not always use percent-encoding ([#&#8203;1738](TypeStrong/ts-node#1738), [#&#8203;1726](TypeStrong/ts-node#1726), [#&#8203;1729](TypeStrong/ts-node#1729)) [@&#8203;cspotcode](https://github.com/cspotcode)
-   Fixed bug where v8-compile-cache-lib did not correctly unhook itself ([#&#8203;1717](TypeStrong/ts-node#1717), [#&#8203;1718](TypeStrong/ts-node#1718), [#&#8203;1719](TypeStrong/ts-node#1719)) [@&#8203;cspotcode](https://github.com/cspotcode)
    -   This internal dependency is used to speed up loading the TypeScript compiler

**Docs**

-   Many docs improvements ([#&#8203;1682](TypeStrong/ts-node#1682)) [@&#8203;cspotcode](https://github.com/cspotcode)
-   Options page: each option its own linkable header w/usage example ([#&#8203;1606](TypeStrong/ts-node#1606)) [@&#8203;cspotcode](https://github.com/cspotcode)
-   Categorize APIs in typedoc, make entrypoints more prominent ([#&#8203;1456](TypeStrong/ts-node#1456)) [@&#8203;cspotcode](https://github.com/cspotcode)
-   Clarify that the shorthand for `--project` is `-P`, not `-p` ([#&#8203;1731](TypeStrong/ts-node#1731), [#&#8203;1734](TypeStrong/ts-node#1734)) [@&#8203;lobsterkatie](https://github.com/lobsterkatie)
-   Add common ESM errors to Troubleshooting page ([#&#8203;1607](TypeStrong/ts-node#1607)) [@&#8203;cspotcode](https://github.com/cspotcode)

https://github.com/TypeStrong/ts-node/milestone/12

</details>

---

### Configuration

📅 **Schedule**: 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 <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1367
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
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