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

Not working on TS 5.0 #140

Open
samchon opened this issue Feb 5, 2023 · 17 comments
Open

Not working on TS 5.0 #140

samchon opened this issue Feb 5, 2023 · 17 comments

Comments

@samchon
Copy link
Contributor

samchon commented Feb 5, 2023

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

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

samchon commented Feb 17, 2023

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

samchon commented Feb 17, 2023

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

microsoft/TypeScript#52826

@jakebailey
Copy link

jakebailey commented Feb 17, 2023

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

is there a workaround to this?

@nonara
Copy link
Contributor

nonara commented Mar 29, 2023

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

sunrabbit123 commented Mar 29, 2023

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

nonara commented Mar 29, 2023

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

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

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

@nonara
Copy link
Contributor

nonara commented Mar 29, 2023

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

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

nonara commented Mar 29, 2023

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/[email protected]/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
Copy link

sunrabbit123 commented Mar 30, 2023

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/[email protected]/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
Copy link

sunrabbit123 commented Mar 30, 2023

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

nonara commented Apr 10, 2023

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

@nonara works for me! 👍

bockstaller added a commit to bockstaller/gauge-ts that referenced this issue Oct 31, 2023
- switches ts-jest compiler from ttypescript to ts-patch due to cevek/ttypescript#140
- switches to typescripts getDecorators() method as .decorator is removed
- uses new declare global syntax https://www.typescriptlang.org/docs/handbook/declaration-files/templates/global-modifying-module-d-ts.html
- uses typescript factory.getParameterDeclaration
- fixes types in tests

Signed-off-by: Lukas Bockstaller <[email protected]>
alanbsmith pushed a commit to Workday/canvas-kit that referenced this issue Oct 14, 2024
Fix canary and release builds that broke with TypeScript 5.0: cevek/ttypescript#140

Switched build compiler to `ts-patch` instead.

[category:Infrastructure]
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

No branches or pull requests

7 participants