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

swc transpiler returns undefined after first value is echoed #1478

Closed
wants to merge 1 commit into from

Conversation

cranberyxl
Copy link
Contributor

@cranberyxl cranberyxl commented Sep 28, 2021

This test passes w/o the --transpiler argument. I',m not sure if it's an swc or ts-node bug. I'll dig around, but maybe someone can point me in the right direction?

EDIT: Merged as part of #1602

@cspotcode
Copy link
Collaborator

When I need to debug the compiler's output, I open up node_modules/ts-node/dist/index.js or node_modules/ts-node/dist/repl.js, find where it invokes the .compile() function, and add a console.log to inspect the output. It's not pretty, but it's quick and it works well enough for the time being.

@cspotcode
Copy link
Collaborator

The way our repl works is that we pretend that every line of input goes into one typescript file, as if you had been entering those lines one after the other in an editor. Then we compile it, and then we use a diff library to figure out which lines of the compiled output are new. We assume that the new ones need to be eval'ed and we do that. Maybe the diffing is going wrong? Just a guess

@cspotcode
Copy link
Collaborator

I think I figured it out.

Somehow, the output from swc includes a trailing newline.

The diffing algorithm reports this change with swc:

{ count: 1, added: true, removed: undefined, value: '401;\n' }
{ count: 1, value: '\n' }
{
  count: 1,
  added: undefined,
  removed: true,
  value: '//# sourceMappingURL=data:application/json;charset=utf-8;base64,<redacted>'
}
{
  count: 1,
  added: true,
  removed: undefined,
  value: '//# sourceMappingURL=data:application/json;charset=utf-8;base64,<redacted>'
}

This is a tiny bit different from the diff we get without swc:

{
  count: 1,
  added: undefined,
  removed: true,
  value: '//# sourceMappingURL=data:application/json;charset=utf-8;base64,<redacted>'
}
{
  count: 2,
  added: true,
  removed: undefined,
  value: '401;\n' +
    '//# sourceMappingURL=data:application/json;charset=utf-8;base64,<redacted>'
}

I'm guessing that, somehow, that tiny difference affects the REPL's behavior. And if you can figure that out, maybe we can reliably avoid this bug by trimming newlines or something.

@cspotcode
Copy link
Collaborator

We take every "added: true" item in the diff and eval each one after the other. I believe we only return the evaluation result of the last eval. In the swc case, the diff has 2 additions: one with 401; and the other with the sourcemap comment.

When the sourcemap comment is eval-ed alone it evals to undefined, meaning with swc, you get undefined in the REPL when you should be getting 401.

Perhaps the solution is to remove the sourcemap comment before diffing?

@cspotcode cspotcode added the bug label Sep 29, 2021
@codecov
Copy link

codecov bot commented Sep 29, 2021

Codecov Report

Merging #1478 (2e39370) into main (6e6bf63) will not change coverage.
The diff coverage is n/a.

@cspotcode
Copy link
Collaborator

@cranberyxl think you'll have time for a pull request? This should be a straightforward change; just remove any trailing sourcemap comment before diffing in the REPL. Probably can be done with a regexp.

@cranberyxl
Copy link
Contributor Author

I can't figure out how to run this stuff locally or where it's happening in the code. Is there some documentation on that? Is there a way to run a single test?

@cspotcode
Copy link
Collaborator

cspotcode commented Oct 10, 2021 via email

@cspotcode
Copy link
Collaborator

I've merged VSCode debug configurations to the main branch. If you prefer to use a debugger, opening a .spec.ts file and pressing "F5" should "just work" and hit breakpoints that you've set in that file.

@cspotcode
Copy link
Collaborator

@cranberyxl any luck putting together a PR for this?

@cspotcode cspotcode added this to the next milestone Jan 21, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Feb 16, 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.4.0` -> `10.5.0`](https://renovatebot.com/diffs/npm/ts-node/10.4.0/10.5.0) |

---

### Release Notes

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

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

[Compare Source](TypeStrong/ts-node@v10.4.0...v10.5.0)

<!--
  I don't make a discussion thread for every release.  Github has a button to make a discussion thread for a release.
  Then I update the discussion thread to remove the release notes and instead link to the release.
-->

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

**Added**

-   Eliminate "Emit Skipped" errors ([#&#8203;693](TypeStrong/ts-node#693), [#&#8203;1345](TypeStrong/ts-node#1345), [#&#8203;1629](TypeStrong/ts-node#1629))
    -   Avoids all "Emit Skipped" errors by performing a fallback `transpileOnly`-style transformation.
    -   Does not affect typechecking.  Type errors are still detected and thrown.
    -   Fallback has the same limitations as `isolatedModules`. This will only affect rare cases such as using `const enums` with `preserveConstEnums` disabled.
    -   Fixes [#&#8203;693](TypeStrong/ts-node#693)
-   Graduate swc transpiler out of experimental; add `swc: true` convenience option ([docs](https://typestrong.org/ts-node/docs/transpilers)) ([#&#8203;1487](TypeStrong/ts-node#1487), [#&#8203;1536](TypeStrong/ts-node#1536), [#&#8203;1613](TypeStrong/ts-node#1613), [#&#8203;1627](TypeStrong/ts-node#1627))
    -   `"swc": true` or `--swc` will use swc for faster execution
    -   This feature is no longer marked "experimental."  Thank you to everyone who filed bugs!
-   swc transpiler attempts to load `@swc/core` or `@swc/wasm` dependencies from your project before falling-back to global installations ([#&#8203;1613](TypeStrong/ts-node#1613), [#&#8203;1627](TypeStrong/ts-node#1627))
    -   global fallback only occurs when using a global installation of ts-node
-   Add support for TypeScript's `traceResolution` output ([docs](https://www.typescriptlang.org/tsconfig/#traceResolution)) ([#&#8203;1128](TypeStrong/ts-node#1128), [#&#8203;1491](TypeStrong/ts-node#1491)) [@&#8203;TheUnlocked](https://github.com/TheUnlocked)
-   Support import assertions in ESM loader ([docs](https://nodejs.org/dist/latest-v17.x/docs/api/esm.html#import-assertions)) ([#&#8203;1557](TypeStrong/ts-node#1557), [#&#8203;1558](TypeStrong/ts-node#1558), [#&#8203;1559](TypeStrong/ts-node#1559), [#&#8203;1573](TypeStrong/ts-node#1573)) [@&#8203;Pokute](https://github.com/Pokute), [@&#8203;geigerzaehler](https://github.com/geigerzaehler)
    -   Allows importing JSON files from ESM with the requisite flag ([docs](https://nodejs.org/dist/latest-v17.x/docs/api/esm.html#json-modules))
-   `ts-node -vvv` also logs absolute paths to `ts-node` and `typescript`, to make it more obvious when you're accidentally using globally-installed versions ([#&#8203;1323](TypeStrong/ts-node#1323), [#&#8203;1620](TypeStrong/ts-node#1620))
-   Add swc target "es2022" ([#&#8203;1535](TypeStrong/ts-node#1535), [#&#8203;1540](TypeStrong/ts-node#1540))
    -   When you have target es2022 in tsconfig, will use swc's es2022 target

**Changed**

-   Initialize TypeScript compiler before starting REPL prompt ([#&#8203;1498](TypeStrong/ts-node#1498)) [@&#8203;TheUnlocked](https://github.com/TheUnlocked)
    -   Improves responsiveness for first line of REPL input
-   Use `v8-compile-cache-lib` to load typescript
    -   improves startup time ([#&#8203;1339](TypeStrong/ts-node#1339), [#&#8203;1603](TypeStrong/ts-node#1603))
-   Support both `--camelCase` and `--hyphen-case` for all CLI flags; update documentation to use `--camelCase` ([#&#8203;1598](TypeStrong/ts-node#1598), [#&#8203;1599](TypeStrong/ts-node#1599))
    -   Not a breaking change; CLI continues to accept both forms
-   Make `TSError` `diagnosticText` property non-enumerable to prevent it from being logged below the stack ([#&#8203;1632](TypeStrong/ts-node#1632))

**Fixed**

-   Fix [#&#8203;1538](TypeStrong/ts-node#1538): REPL inputs fail to transpile via swc ([#&#8203;1538](TypeStrong/ts-node#1538), [#&#8203;1541](TypeStrong/ts-node#1541), [#&#8203;1602](TypeStrong/ts-node#1602))
-   Fix [#&#8203;1478](TypeStrong/ts-node#1478): REPL erroneously logged `undefined` for all inputs after the first when using swc transpiler ([#&#8203;1478](TypeStrong/ts-node#1478), [#&#8203;1580](TypeStrong/ts-node#1580), [#&#8203;1602](TypeStrong/ts-node#1602))
-   Fix [#&#8203;1389](TypeStrong/ts-node#1389): In `--showConfig` output, emit accurate `moduleTypes` paths resolved relative to the `tsconfig.json` which declared them ([#&#8203;1389](TypeStrong/ts-node#1389), [#&#8203;1619](TypeStrong/ts-node#1619))
-   Fix: Remove indentation from `ts-node --help` output ([#&#8203;1597](TypeStrong/ts-node#1597), [#&#8203;1600](TypeStrong/ts-node#1600))
-   Fix [#&#8203;1425](TypeStrong/ts-node#1425): Merged definitions correctly into `tsconfig.schemastore-schema.json` ([#&#8203;1425](TypeStrong/ts-node#1425), [#&#8203;1618](TypeStrong/ts-node#1618))
-   Fix: Allow disabling `"use strict"` emit in SWC transpiler ([#&#8203;1531](TypeStrong/ts-node#1531), [#&#8203;1537](TypeStrong/ts-node#1537))
-   Fix: Add missing `ERR_UNKNOWN_FILE_EXTENSION` constructor; was throwing `ERR_UNKNOWN_FILE_EXTENSION is not a constructor` ([#&#8203;1562](TypeStrong/ts-node#1562)) [@&#8203;bluelovers](https://github.com/bluelovers)
-   Fix [#&#8203;1565](TypeStrong/ts-node#1565): entrypoint resolution failed on node v12.0.x and v12.1.x ([#&#8203;1565](TypeStrong/ts-node#1565), [#&#8203;1566](TypeStrong/ts-node#1566)) [@&#8203;davidmurdoch](https://github.com/davidmurdoch)

#### Docs

-   Explain `env -S` flag for shebangs ([docs](https://typestrong.org/ts-node/docs/usage#shebang)) ([#&#8203;1448](TypeStrong/ts-node#1448), [#&#8203;1545](TypeStrong/ts-node#1545)) [@&#8203;sheeit](https://github.com/sheeit), [@&#8203;chee](https://github.com/chee)
-   Suggest `skipIgnore` when you want to compile files in node_modules ([docs](https://typestrong.org/ts-node/docs/how-it-works)) ([#&#8203;1553](TypeStrong/ts-node#1553)) [@&#8203;webstrand](https://github.com/webstrand)
-   Fix typo in `moduleTypes` on options page ([docs](https://typestrong.org/ts-node/docs/options)) ([#&#8203;1630](TypeStrong/ts-node#1630), [#&#8203;1633](TypeStrong/ts-node#1633))

#### Misc

-   Adds experimental `experimentalResolverFeatures` option, but it does not do anything yet ([#&#8203;1514](TypeStrong/ts-node#1514), [#&#8203;1614](TypeStrong/ts-node#1614))

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

</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 <[email protected]>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1156
Reviewed-by: crapStone <[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
bug you can do this Good candidate for a pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants