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

test(custom-compiler): deprecated ttypescript and use ttsc #4074

Closed
wants to merge 2 commits into from

Conversation

sunrabbit123
Copy link

Summary

Less frequent updates to ttypescript and less attention to maintenance.
That's why we recommend the following libraries ttsc

And this support TS@5

Test plan

With node@16.19.1
image

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4549846699

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 95.81%

Totals Coverage Status
Change from base Build 4535368977: -0.02%
Covered Lines: 4055
Relevant Lines: 4154

💛 - Coveralls

@sunrabbit123
Copy link
Author

Do I need to modify the legacy side as well?
I think that's out of my scope of work.

@kulshekhar

@ahnpnl
Copy link
Collaborator

ahnpnl commented Mar 29, 2023

Yes pls do because that is the codes currently running, even though it’s called legacy but it is the actual main source codes

@nonara
Copy link

nonara commented Mar 29, 2023

This will not work. See below.

@sunrabbit123
Copy link
Author

Make sure to present solid evidence and arguments in the linked issue.
Don't just make unsubstantiated claims here and there.

If you have enough evidence, I will withdraw the promotion.

@kulshekhar
Copy link
Owner

I'll be diving into the relevant conversations, which seem to be spread across repos, after work today to get an understanding of the situation.

Let's please hold off on merging until then.

@nonara
Copy link

nonara commented Mar 31, 2023

@kulshekhar fwiw, we expect to release the new version of tsp next week, which will also have drop-in replacement for ttsc. Only difference for migration purposes is the command: which will be tspc. I am also planning to do a write-up that explains why the prior approach started in ttypescript (which tsp was originally based on) is largely unsound and how it became more unsound with the new ts version.

This debacle is a great illustration of that, unfortunately. We have regex replace doing code manipulation, which is bad enough. There is also no throw if it fails, leading to situations like this where people think it's working because it doesn't throw.

The proper and most future-proof solution is through AST manipulation, which we've implemented. The new ts5 compilation method and output structure essentially requires this for various reasons. To mitigate perf differences, we've also introduced caching, which should ultimately improve speed over former tsp and ttypescript after the first run.

Any other questions, feel free to ask. Sorry for the drama making it to your inbox!

@sunrabbit123
Copy link
Author

sunrabbit123 commented Mar 31, 2023

Dear ts-jest developers, I apologize for having this conversation here. Please feel free to ignore this comment.

And if the structure changes further as the TS version goes up, that ttsc will most likely not work (it will be broken).
So if you don't want to deal with that issue again in the future, you might want to test your custom-compiler with that library after the ts-patch is released.

The context is going to be a lot scattered.
It probably looks like this

  1. ttypescript(2 issue)
  2. ts-patch(1 issue)

Dear nonara

My language was a bit harsh.
I apologize for that.

And I'm guessing it probably has some impact on running the translator as well (which sounds like an excuse).

BTW the reason I referred to myself as stupid was literally because I didn't think the code in question had much of an impact on the product's behavior, so I didn't understand what problem you were talking about based on what you said.
It was a rationalization of my understanding, not an emotional response.
Also, I don't see where I was being abusive.
That probably comes from my own ignorance.

And I'm not talking about "unreliable".

When I accepted and understood your story.

the code you gave me was taken out of context.
And I know that trashing actually removes a lot of code.

Perhaps you misunderstood my purpose.

I know the code is bad, and I know it's a workaround, because I wrote it.
I know it's bad, and I know it's a workaround, and I know it can break things that I didn't write.
But what I've been saying all along is that this is code that works right now, and when I've presented the library elsewhere, I've said "ts-patch is the best way to go, but you can use this as a stopgap". (I left out the first two or three places, that's my fault).

Nevertheless, I tried to get the existing test cases to pass and with the exception of ts-node, the rest of them passed on TS@5 and above.

I also took your advice and fixed the issue with the version checking I was doing in that part.

When I said "Please be precise and specific At least that the test case is broken.", I meant to say that if there is a problematic transformer, please let me know about it.

As for the "5 minutes ago" comment, I don't know where you got that number from, but it's certainly a shorter period of time than yours, and the mitigation was inadequate.
As jake mentioned, modifying runtime code is likely to break.
Especially the more frequently the TS changes.

So from a troubleshooting perspective, my approach may seem pathetic and stupid.
And it certainly does to me.

I'll leave it at that.

PS.
I apologize if I've come across as arrogant and hostile.
I just want you to know that it wasn't my intention.
English is still awkward for me.

The comment in the link below are rude and show hostility. I guess I got carried away.
cevek/ttypescript#140 (comment)

@nonara
Copy link

nonara commented Mar 31, 2023

@sunrabbit123 I appreciate that, and I don't think you're stupid. In earlier conversation, I tried to be kind, but my patience wore thin, when what I was saying was not accepted and was met with some hostility. That being said, I apologize also for anything I wrote which may have demonstrated irritability.

The one thing on this is that in some messages you're claiming that this is a temporary solution that works in some cases. I told you that was fine, so long as it was made clear to users. In others places, however, you presented it as "competition" and a working solution. My main concern was that people understood this wasn't going to work as it's intended.

As a maintainer in this space trying to unify and solve the issue of lagging support in the other library, seeing my code with a very important line broken being shipped and attempted to be merged into major libraries as a replacement library was concerning to me.

In any event, thank you again for your message, and know that I don't think you're stupid or that your solution is "pathetic". All I wanted to do was to help you to understand that there was more to the problem.

Best,
Ron

PS. In respect for the others being emailed on this personal conversation, you're welcome to email me if you wanted to respond.

@sunrabbit123
Copy link
Author

The one thing on this is that in some messages you're claiming that this is a temporary solution that works in some cases. I told you that was fine, so long as it was made clear to users. In others places, however, you presented it as "competition" and a working solution. My main concern was that people understood this wasn't going to work as it's intended.

I'm guessing it's a mix of contexts that led to the miscommunication.

Also, I feel bad for the ts-jest developers for bringing this up here.
Since our conversation, I think it's better to talk about this custom-compiler stuff through the following URL.
cevek/ttypescript#140

I don't think it's good for misunderstandings to build up, so I'll just summarize my side of the story.
Let me summarize where I think the miscommunication came from.

  1. when you said "Creating another fork is not good imo.", I thought you were talking about "competition".
    So I responded to the conversation underneath by saying "I'm not interested in extending features."

  2. var StatisticType was a part of my ttsc library that was not even touched in TS@5.
    The ternary operator in question requires a major of at least 4 and a minor of at least 9, so the
    It is not touched in any TS@5 release to date.
    So I didn't understand what you were trying to say.

  3. structural differences and issues like this are something I'm aware of, and I'm expect we have different ideas of what we mean by "a working library" and "a library that is maintained and contributes to the ecosystem".

@kulshekhar
Copy link
Owner

@sunrabbit123 @nonara thank you both for bringing the conversation back to a respectable tone. It's easy for conversations to get out of hand especially in a remote/online context so I want to commend you both for the way you've brought tempers back down. Thank you.

I've gone through the comments here and elsewhere to gain a little bit of an understanding based on which, it sounds like the updates in the newly proposed library won't work when tsc is updated to v5. I've only read through these once so if I'm mistaken, please let me know.

However, if this understanding is correct, I propose closing this PR for now.

@sunrabbit123
Copy link
Author

While this works for now, it may become unworkable in the future as the TS structure changes.

Since you probably don't want this, let's close it.

@nonara
Copy link

nonara commented Jun 13, 2023

@kulshekhar Just a heads up — tsp v3 is now released and should work here. You can simply use tspc instead of ttsc

@sunrabbit123 sunrabbit123 deleted the support/ts5 branch June 14, 2023 06:08
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 this pull request may close these issues.

5 participants