cevek / ttypescript

Over TypeScript tool to use custom transformers in the tsconfig.json
1.53k stars 56 forks source link

Not working on TS 5.0 #140

Open samchon opened 1 year ago

samchon commented 1 year ago

When run ttsc command, below error occurs:

D:\github\samchon\typia\node_modules\ttypescript\lib\patchCreateProgram.js:84
    tsm.createProgram = createProgram;
                      ^

TypeError: Cannot set property createProgram of #<Object> which has only a getter
    at patchCreateProgram (D:\github\samchon\typia\node_modules\ttypescript\lib\patchCreateProgram.js:84:23)
    at loadTypeScript (D:\github\samchon\typia\node_modules\ttypescript\lib\loadTypescript.js:21:56) 
    at Object.<anonymous> (D:\github\samchon\typia\node_modules\ttypescript\lib\tsc.js:8:46)
    at Module._compile (node:internal/modules/cjs/loader:1226:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1280:10)
    at Module.load (node:internal/modules/cjs/loader:1089:32)
    at Module._load (node:internal/modules/cjs/loader:930:12)
    at Module.require (node:internal/modules/cjs/loader:1113:19)
    at require (node:internal/modules/cjs/helpers:103:18)
    at Object.<anonymous> (D:\github\samchon\typia\node_modules\ttypescript\bin\tsc:2:1)
szatanjl commented 1 year ago

Don't know if it is related but I was looking through the code and I find the commit f37f095 suspicious.

It adds check if tsc version is >= 4.9 (I believe that was the intent), but does so incorrectly major >= 4 && minor >= 9 @ tsc.ts

samchon commented 1 year ago

You're right, I did a mistake on the version specification.

However, TS 5.0 breaks ttypescript too, even when fixing the version checking error.

The error comes from tsm.createProgram became a readonly proeprty.


import ts from "typescript";

console.log(ts.createProgram);
ts.createProgram = function () {} as any;
TypeError: Cannot set property createProgram of #<Object> which has only a getter
samchon commented 1 year ago

This problem is based on TS 5.0 has changed target build option from es5 to es2018.

Such break change invalidates key logic of ttypescript.

I wrote an issue about that in TypeScript repo, but I think ttypescript needs to ready for that - @cevek

https://github.com/microsoft/TypeScript/issues/52826

jakebailey commented 1 year ago

@samchon This problem has nothing to do with the target being changed. TypeScript is now itself authored in modules, which do not allow at-runtime modification of exports, as opposed to previous versions which used TS namespaces, which are just objects and could be modified (though any modification like that would be very, very fragile as there's no guarantee the exported property would be used internally).

damiangreen commented 1 year ago

is there a workaround to this?

nonara commented 1 year ago

Here's a quick update for everyone - the changes in v5 required a new approach for a few reasons, but we have it solved and are about to release a new version of ts-patch.

We've opted to add drop-in replacement support for ttypescript which has had lagging maintenance support for a while now.

What this means is, you can install ts-patch and simply use tspc, the same as you would use ttsc. It will be on-the-fly, in memory modification, although it's notably faster due to our caching mechanism.

We're nearly finished, so you can expect this soon.

sunrabbit123 commented 1 year ago

@damiangreen If you don't have time to migrate, enter

> yarn remove ttypescript
> yarn add -D ttsc

If so, your project will work just fine as is. Unless you're only using ts-node and cache features.

If you have enough time to migrate, we recommend using ts-patch.

nonara commented 1 year ago

For clarity, the whole point of "drop-in-support" means you don't have to "migrate". You simply install and use tspc instead of ttsc.

Creating another fork is not good imo.

@sunrabbit123 last I saw, your solution lacks a critical piece, which will strip transformers of the full functionality that many will need, so you should be aware of that.

There is more to this than simply patching createProgram, for tsc.js

sunrabbit123 commented 1 year ago

If so, it's simply a library that only works for now, so the Isn't that a good thing for you, who only want one library?

I don't have a big picture, I just developed and deployed what I needed for the moment. @nonara

sunrabbit123 commented 1 year ago

In case you're wondering, the I'm not interested in extending the functionality. I'm just concerned with typescript version compatibility.

nonara commented 1 year ago

In general, I believe not forking is generally the ideal path, if it can be done, but there are certainly times which call for it. I just tend to take the long view in that forks (or any new project) should be planned carefully and done with the intention of maintaining it for the long term, for as long as people rely on it.

With that said, it's OSS, so it definitely isn't for me to tell others what to do. I just wanted to voice my thoughts on it and also to let you know of the potential issues many will face with the approach in your PR. It's an issue we've faced and had to solve for over the years with both tts and tsp.

That being said, I know people are anxious, so if your solution works for their particular plugins without any issue, I think it's great that they have something to use in the mean time!

sunrabbit123 commented 1 year ago

Numerous forks clutter the ecosystem, but they also make it mature.

Competition is necessary for better products, As a company owner, I'm sure you know the benefits of having competition.

Of course, from the user's point of view, as you say, it's inconvenient and unsettling when the product, the interface, is not fixed. That's what you're concerned about.

That's why I'm talking about it. I'm not interested in extending features.

So any transpiler that works with TTSC will work with TSP. Of course, it may not be the opposite.

nonara commented 1 year ago

So, one point here. I see that you're going out and opening PRs in major libraries (ts-jest). I also just looked at your code, and I can confirm that it isn't going to work. I'll detail below.

I think it's prudent for you to understand the scope of the problem and get your approach right before you start opening PRs. At the very least, let users know that it's not a complete solution.

Look at this file: https://github.com/sunrabbit123/ttypescript/blob/master/packages/ttypescript/src/tsc.ts#L15

I wrote the solution you see there to accommodate 4.9. That simply will not work with v5.

Look in https://unpkg.com/typescript@5.0.2/lib/tsc.js and search for "var StatisticType". It isn't in the file. Nor would it be.

If you want to confirm, run your code in the debugger and tell me if that replace does anything. If it doesn't, it won't work properly. If it does, it wouldn't even run, because it'd be broken due to the structural differences of v4 and v5. There is a reason we're still working on this. It's not a trivial fix.

Competition is fine — what bugs me is when someone is trying to inject an unworking solution into larger tools without an understanding of how it even works.

There is a reason Cevek and I have the lines we do regarding tsc. It's clear that you don't understand why we did it and that you don't understand that your code is broken. If you want to do this responsibly, I'd ask that you let people know that your fork is not complete and will break in many situations, and please do not try to submit PRs to libraries until you've understood the problem and solved for it.

If you want to take it on, you can find some detailed explanations that I've written on what we're doing and why in the open issues on ts-patch, including a discussion with the MSFT engineer who made the changes in v5.

Simply put, please get a handle on the issue and submit a complete solution before trying to submit PRs to major tools that we all rely on.

Or, you can simply wait for tsp, which is, as it's always been, backwards compatible with ttsc.

sunrabbit123 commented 1 year ago

Thanks for the advice But what the hell are you talking about I don't understand your reference

Look at this file: sunrabbit123/ttypescript@master/packages/ttypescript/src/tsc.ts#L15 I wrote the solution you see there to accommodate 4.9. That simply will not work with v5. Look in unpkg.com/typescript@5.0.2/lib/tsc.js and search for "var StatisticType".

let commandLineTsCode = fs
    .readFileSync(tscFileName, 'utf8')
    .replace(
        major == 4 && minor >= 9
            ? /^[\s\S]+(\(function \(ts\) {\s+var StatisticType;[\s\S]+)$/
            : /^[\s\S]+(\(function \(ts\) {\s+function countLines[\s\S]+)$/,
        '$1'
    );
if (major >= 5) {
    commandLineTsCode = commandLineTsCode.replace(/\= createProgram\(/g, '= ts.createProgram(');
}

It isn't in the file. Nor would it be.

If you have a plugin that doesn't currently work with TTSC, please bring it up and tell us about it. I'm too dumb to understand your reference In 5.0.2, var StaticType is neither found nor touched This means that 5.0.2 does not correspond to major == 4 && minor >= 9

Ok, sir I'm v4 and my minor should be 9 or higher.

I have the wrong category But that's not the immediate problem

Fixed it this version

sunrabbit123 commented 1 year ago

Bring me a specific case of something not working You have not properly proposed the code in question, nor the problem.

Please be precise and specific At least that the test case is broken.

I'm not trying to be stubborn, I'm just not convinced and don't understand.

And

please get a handle on the issue and submit a complete solution before trying to submit PRs to major tools that we all rely on.

The only thing I added to ts-jest was a test file I didn't modify any logic.

Please don't drag in unrelated stories. And look at the modified code at least once before you talk about problems.


Comments

I think I was being a bit harsh at the time. I apologize if it made you uncomfortable.

nonara commented 1 year ago

Hi all!

We have a beta ready for testing. It can directly replace ttypescript, using the same in-memory modification technique. Very easy to switch

Details here:

Let me know if it works for you.

mindplay-dk commented 1 year ago

@nonara works for me! 👍