tapjs / tsimp

https://tapjs.github.io/tsimp/
Other
508 stars 12 forks source link

tsimp does not work with TypeScript v5.6.2 #29

Closed gfx closed 1 week ago

gfx commented 1 month ago

Error messages in .tsimp/daemon/log:

TypeError: Cannot read properties of undefined (reading 'moduleResolution')
    at computeValue (/home/goro/ghq/github.com/gfx/typescript-cookbook/node_modules/typescript/lib/typescript.js:21622:46)
    at importSyntaxAffectsModuleResolution (/home/goro/ghq/github.com/gfx/typescript-cookbook/node_modules/typescript/lib/typescript.js:21599:28)
    at getDefaultResolutionModeForFileWorker (/home/goro/ghq/github.com/gfx/typescript-cookbook/node_modules/typescript/lib/typescript.js:127780:10)
    at Object.getMode (/home/goro/ghq/github.com/gfx/typescript-cookbook/node_modules/typescript/lib/typescript.js:124407:85)
    at Object.resolveTypeReferenceDirectiveReferences (file:///home/goro/ghq/github.com/gfx/typescript-cookbook/node_modules/tsimp/dist/esm/service/resolve-type-reference-directive-references.js:24:45)
    at resolveTypeReferenceDirectiveNamesWorker (/home/goro/ghq/github.com/gfx/typescript-cookbook/node_modules/typescript/lib/typescript.js:125232:20)
    at resolveNamesReusingOldState (/home/goro/ghq/github.com/gfx/typescript-cookbook/node_modules/typescript/lib/typescript.js:125354:14)
    at resolveTypeReferenceDirectiveNamesReusingOldState (/home/goro/ghq/github.com/gfx/typescript-cookbook/node_modules/typescript/lib/typescript.js:125325:12)
    at processTypeReferenceDirectives (/home/goro/ghq/github.com/gfx/typescript-cookbook/node_modules/typescript/lib/typescript.js:126706:156)

It works with older TypeScript compilers, so it might be a regression in TypeScript.

mattdean-digicatapult commented 1 month ago

It looks to me like this was broken with https://github.com/microsoft/TypeScript/pull/58825 which modified the implementation of the getMode method of typeReferenceResolutionNameAndModeGetter to take a third parameter of compilerOptions. I would not however consider this a typescript bug at first glance because the ResolutionNameAndModeGetter<FileReference | string, SourceFile | undefined> interface that the object implements has previously had getMode with that additional parameter.

I think the solution is just to pass compilerOptions in at the call to this method https://github.com/tapjs/tsimp/blob/a2e9aefcbf2fa65554bc75057ee7e9032f06ed90/src/service/resolve-type-reference-directive-references.ts#L59

I'll try to put up a PR and see if the maintainers here agree

FedericoBiccheddu commented 1 month ago

Thank you @mattdean-digicatapult.

Until the PR get merged, I'm patching locally using the following patch with pnpm:

diff --git a/dist/esm/service/resolve-type-reference-directive-references.js b/dist/esm/service/resolve-type-reference-directive-references.js
index d285ff041190ba2cf2958b00831c089e175fd788..9f33eebec9a498afd0455331ba7aa10d7aa0de89 100644
--- a/dist/esm/service/resolve-type-reference-directive-references.js
+++ b/dist/esm/service/resolve-type-reference-directive-references.js
@@ -21,7 +21,7 @@ export const getResolveTypeReferenceDirectiveReferences = (host, moduleResolutio
         const loader = createLoader(containingFile, redirectedReference, options, host, resolutionCache);
         for (const entry of entries) {
             const name = loader.nameAndMode.getName(entry);
-            const mode = loader.nameAndMode.getMode(entry, containingSourceFile);
+            const mode = loader.nameAndMode.getMode(entry, containingSourceFile, options);
             const key = createModeAwareCacheKey(name, mode);
             let result = rtrdrInternalCache.get(key);
             if (!result) {
krutoo commented 1 month ago

same issue

stuft2 commented 1 month ago

fwiw, this bug makes the node test runner appear to pass the tests when they should fail.

// fail.ts
import { test } from 'node:test'
import { strict as assert } from 'node:assert'

test('should fail', t => {
    assert.fail('should fail')
})

running the following command

# Node.js v22.9.0
node --import=tsimp/import --test fail.ts

outputs

✔ fail.ts (1365.779513ms)
ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 1379.271959

EDIT: it appears to fail silently on all usages, not just in the test runner.

gfx commented 1 month ago

Hi, @isaacs,

Can you take a look at this issue? This issue has prevented me from upgrading TypeScript.

NoahAndrews commented 1 month ago

He's shown zero interest in this project recently (as is his right). Until that changes, the only sustainable path forward for us is to fork and maintain it ourselves, or to move to another solution (probably tsx with a separate type-checking step).

Even before this issue occurred, there were severe caching issues that often required killing the daemon just to get code changes to apply. This package is simply not production-ready yet (though I really hope it gets there one day, whether by @isaacs or someone else).

I took a stab at fixing the caching issues, but it didn't seem to work: https://github.com/tapjs/tsimp/pull/22

isaacs commented 1 month ago

Yeah, this is definitely somewhat stalled at the "proof of concept" stage. If someone wanted to pick up the baton and try to get it over the finish line (or some other kind of sports metaphor?) I'd love to see it and would be happy to collaborate or smoke-test as I have time to. Startup work is taking up all my time right now, which unfortunately doesn't leave much for tsimp, despite it being (I think) a promising idea.

krutoo commented 1 month ago

@isaacs The most acute problem now is the impossibility of using new versions of TypeScript together with tsimp

Could we merge PR and publish a patch release for this?

The second problem in my opinion is related to the caching features that @NoahAndrews spoke about

gfx commented 4 weeks ago

@isaacs Okay, I will maintain tsimp for a while. Could you add me (@gfx) as a maintainer for the repository and npm module?

I am also working for a startup and thus have less time to contribute, but because I need tsimp for my side work (FWIW, I'm writing a book on TypeScript and introducing tsimp as the interpreter for TypeScript), I really want it to work well for the latest TypeScript.

My npmjs account is https://www.npmjs.com/~gfx

NoahAndrews commented 4 weeks ago

tsimp is absolutely not ready for recommendation in a book (unless maybe you're also wanting to fix the caching issues?)

gfx commented 4 weeks ago

@NoahAndrews lol. It's off-topic, but IIUC tsimp is virtually the only tool that supports TypeScript type checking and compiling. I used to use ts-node, but it doesn't work with ES modules, and I think it's too huge to maintain -- there are about 200 open issues on the repo. Instead, tsimp just works for me (except for TypeScript 5.6 or higher).

NoahAndrews commented 4 weeks ago

You can also just call tsc --noemit && tsx. Sure it's not as nice, but at least it's reliable, unlike tsimp, which can give you stale results in non-obvious ways.

laug commented 3 weeks ago

Another idea to consider is using Deno 2.0. It has TypeScript support built-in (including actual type checking, unlike things like tsx) and is supposedly fully compatible with Node, so it could be a good alternative. I agree ts-node is dead though.

isaacs commented 3 weeks ago

I also have a fork of ts-node that tap uses (which tsimp was intended to replace, and probably would have, had I not gotten distracted by doing another startup and needing to make money which is apparently required just to live, in this capitalist dystopia). Might try using that.

krutoo commented 2 weeks ago

@isaacs can you merge this small PR with 2 lines of code and publish it?

https://github.com/tapjs/tsimp/pull/30

Or maybe provide merge/publish rights to someone...

krutoo commented 1 week ago

@gfx tsimp 2.0.12 with typescript 5.6.3 works for me

Can you confirm that issue solved?

gfx commented 1 week ago

@krutoo It works for me, too! FYI, I had to remove the .tsimp/ directory.

Resolved.