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

tsimp does not work with TypeScript v5.6.2 #29

Closed
gfx opened this issue Sep 11, 2024 · 17 comments
Closed

tsimp does not work with TypeScript v5.6.2 #29

gfx opened this issue Sep 11, 2024 · 17 comments

Comments

@gfx
Copy link

gfx commented Sep 11, 2024

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.

djcsdy added a commit to softwareventures/observe-fs that referenced this issue Sep 11, 2024
v5.6.x is not compatible with tsimp.

tapjs/tsimp#29
@mattdean-digicatapult
Copy link
Contributor

It looks to me like this was broken with microsoft/TypeScript#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

const mode = loader.nameAndMode.getMode(

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

@FedericoBiccheddu
Copy link

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
Copy link

krutoo commented Sep 16, 2024

same issue

@stuft2
Copy link

stuft2 commented Sep 19, 2024

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
Copy link
Author

gfx commented Oct 6, 2024

Hi, @isaacs,

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

@NoahAndrews
Copy link

NoahAndrews commented Oct 6, 2024

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: #22

@isaacs
Copy link
Member

isaacs commented Oct 7, 2024

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
Copy link

krutoo commented Oct 9, 2024

@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
Copy link
Author

gfx commented Oct 11, 2024

@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
Copy link

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

@gfx
Copy link
Author

gfx commented Oct 12, 2024

@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
Copy link

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
Copy link

laug commented Oct 16, 2024

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
Copy link
Member

isaacs commented Oct 16, 2024

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
Copy link

krutoo commented Oct 22, 2024

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

#30

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

@krutoo
Copy link

krutoo commented Oct 30, 2024

@gfx tsimp 2.0.12 with typescript 5.6.3 works for me

Can you confirm that issue solved?

@gfx
Copy link
Author

gfx commented Nov 2, 2024

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

Resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants