-
Notifications
You must be signed in to change notification settings - Fork 56
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
Comments
Don't know if it is related but I was looking through the code and I find the commit It adds check if tsc version is >= 4.9 (I believe that was the intent), but does so incorrectly |
You're right, I did a mistake on the version specification. However, TS 5.0 breaks The error comes from import ts from "typescript";
console.log(ts.createProgram);
ts.createProgram = function () {} as any;
|
This problem is based on TS 5.0 has changed target build option from Such break change invalidates key logic of I wrote an issue about that in TypeScript repo, but I think |
@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). |
is there a workaround to this? |
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 We've opted to add drop-in replacement support for What this means is, you can install We're nearly finished, so you can expect this soon. |
@damiangreen > yarn remove ttypescript
> yarn add -D ttsc If so, your project will work just fine as is. If you have enough time to migrate, we recommend using ts-patch. |
For clarity, the whole point of "drop-in-support" means you don't have to "migrate". You simply install and use 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 |
If so, it's simply a library that only works for now, so the I don't have a big picture, I just developed and deployed what I needed for the moment. |
In case you're wondering, the |
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! |
Numerous forks clutter the ecosystem, but they also make it mature. Competition is necessary for better products, 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 why I'm talking about it. So any transpiler that works with TTSC will work with TSP. |
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. |
Thanks for the advice
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. Ok, sir I'm v4 and my minor should be 9 or higher.
Fixed it this version |
Bring me a specific case of something not working Please be precise and specific I'm not trying to be stubborn, I'm just not convinced and don't understand. And
The only thing I added to ts-jest was a test file Please don't drag in unrelated stories. Comments I think I was being a bit harsh at the time. |
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. |
@nonara works for me! 👍 |
- 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 <lukas.bockstaller@posteo.de>
Fix canary and release builds that broke with TypeScript 5.0: cevek/ttypescript#140 Switched build compiler to `ts-patch` instead. [category:Infrastructure]
When run
ttsc
command, below error occurs:The text was updated successfully, but these errors were encountered: