-
Notifications
You must be signed in to change notification settings - Fork 433
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
Remove TypeScript #971
Remove TypeScript #971
Conversation
I'm not really sure if removing TypeScript is the best approach. For me, it really helped with contributing and I feel like it still makes sense to use for library-level code. The DX you get from it is really valuable and actually helps catching bugs. But I also get that it can get annoying really quick in certain situations. But removing the Types altogether (even without JSDocs and/or .d.ts files) is a step back, for both library users and and contributors. |
I will second Marco here. TS is not my main contribution language and I can understand that it makes it harder for a non TS developer to contribute. This being said TS really helped me when I first contributed to Stimulus. And all the work that was done to define all those types is really valuable IMHO. Switching back to JS means :
I am sure they are some rationales behind that move and we would love to hear them. If strict typing is really the issue did you ever consider keeping the TS types with loosened typing rules so that new contribution doesn't have to be fully type |
Have you considered using TSDoc to keep types without needing to compile the codebase with tsc? (Without knowing the rationale for this change, maybe my comment is moot) |
With this change, we'd lose the majority of the benefits we gain with the delegate system that powers the majority of our internal architecture. Defining an Even without introducing types of our own, enforcing the browser-provided types has been extremely valuable. Personally, I'm very much opposed to removing types entirely. What are the main sources of friction that are motivating this change? |
Fully recognize that TypeScript offers some people some advantages, but to my eyes, the benefits are evident in this PR. The code not only reads much better, it's also freed of the type wrangling and gymnastics needed to please the TS compiler. We write all our client side code at 37signals now in pure JavaScript and the same too with any internal libraries. This is going to bring that in line. All the love and appreciation to contributors who would prefer TypeScript. This is one of those debates where arguments aren't likely to move anyone's fundamental position, so I won't attempt to do that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR, created 3h ago and merged 1h ago, seems very rushed.
This PR fundamentally changes the library. It changes the library's main language. I don't see the need to rush this? Why was this rushed?
Was there any prior discussion on this change?
This is an open-source library, not just source available.
There are many other open-source libraries using this library directly or indirectly.
And making this important change rushed, ignoring ALL (and I mean ALL) of the PR comments...sets a precedent.
Is this how Ruby on Rails will be developed as well? On a one man's whim?
Why do we always have to have these arguments? What was so difficult to ask the community (including maintainers), have a constructive discussion, and come up with an agreement whether or not to switch from TS to JS? Or do we just wait for @dhh to make a blog post and then we'll just implement whatever was written there.
You could've better used your time addressing some of the open PRs.
/rant
EDIT: This change here seems to make tests fail:
#971 (comment)
I'm not sure how the tests are passing?
EDIT 2: Seems like the string that was changed is not part of what is being tested, but a part of the error message that will be shown to the developer if the test fails. Thanks @chigia001
sourceType: "module" | ||
}, | ||
rules: { | ||
"comma-dangle": "error", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these changes seem not relevant to the PR title/subject. Why were these introduced in this PR?
"express": "^4.18.2", | ||
"multer": "^1.4.2", | ||
"prettier": "2.6.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come "prettier"
was removed in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is very nice, we dont need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why format or type your code just yolo and hope for the best!
export class ErrorRenderer extends Renderer<HTMLBodyElement, PageSnapshot> { | ||
static renderElement(currentElement: HTMLBodyElement, newElement: HTMLBodyElement) { | ||
export class ErrorRenderer extends Renderer { | ||
static renderElement(currentElement, newElement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We lost information here. currentElement: HTMLBodyElement
makes it obvious this is not any HTML element, but <body>
element. Maybe you could rename currentElement
to currentBodyElement
or add a comment explaining it's not any HTML element?
target: this.formElement, | ||
detail: { formSubmission: this }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was the formatting style changed in this PR? This seems irrelevant.
@@ -251,7 +217,7 @@ export class FormSubmission { | |||
} | |||
} | |||
|
|||
function buildFormData(formElement: HTMLFormElement, submitter?: HTMLElement): FormData { | |||
function buildFormData(formElement, submitter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be useful to know that submitter
is optional argument here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would one express that? The variable now needs to be renamed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pailhead The right way to rewrite the function signature is clearly buildFormData(formElement, submitterWhichIsAnOptionalHTMLElement)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you already forget the only correct, Hungarian Notation? mhtmle_submitter
, where m
is for maybe
, to mimic Haskell style, html
namespaces the complex types present in the browser and e
is for element of course.
/s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, I didn’t know this could be expressed so clearly. Thank you for clarifying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is interesting. I would have no idea what a "submitter's" purpose is here with or without Typescript.
I am excited to see how this code base changes without Typescript. The "types" or what things are will have to be communicated in some way. This may lead to very readable code.
@@ -0,0 +1,2 @@ | |||
export * from "./fetch_request" | |||
export * from "./fetch_response" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file needed? The .ts file was only exporting types. I don't see where this is being used, so this file can probably be removed.
willFollowLinkToLocation(link: Element, location: URL, originalEvent: MouseEvent): boolean { | ||
// Link click observer delegate | ||
|
||
willFollowLinkToLocation(link, location, originalEvent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth noting that originalEvent
is MouseEvent
?
Same for other arguments: link
is Element
, not URL
, which might be confusing now and location
is not window.location
.
Same below.
@@ -65,11 +65,11 @@ test("test form submission with form mode optin and form enabled from submitter | |||
assert.ok(await formSubmitStarted(page)) | |||
}) | |||
|
|||
async function gotoPageWithFormMode(page: Page, formMode: "on" | "off" | "optin") { | |||
async function gotoPageWithFormMode(page, formMode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to document that formMode
could be one of "on"
, "off"
, and "optin"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vfonic how do you know it can be one of those three? It looks like it can be any primitive, object, null or undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pailhead because it was typed like this before 🤷♂️?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the point. That information is now gone, so now it can be anything.
I think they want to rely on tests now only to catch these misuses. Hopefully there is a test for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Wirone it is a large PR and there is no type to narrow it. Without seeing who is calling it I don’t think it’s possible to narrow it down to even a string, let alone these three particular ones. But at best, it feels like it would be one recommendation of how to use it. I can pass Elephant
into it. I probably won’t catch it, my user will.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the point, there was a way to narrow it and now it's gone. It should be at least documented, as @vfonic suggested, to keep this information, even if in less strict form.
@@ -1075,14 +1075,14 @@ test("test following a link with [data-turbo-method] set when html[data-turbo=fa | |||
|
|||
await page.click("#turbo-method-post-to-targeted-frame") | |||
|
|||
assert.equal(await page.textContent("h1"), "Hello", "treats link as a full-page navigation") | |||
assert.equal(await page.textContent("h1"), "Hello", "treats link full-page navigation") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did the tests work before? This is an odd change. Where is this changed in the implementation (outside of test code)?
This seems like a breaking change. It looks like this was made via a global "find TypeScript as XXX
and replace all with '' (empty string)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"treats link as a full-page navigation" is a error message, display when assert fail. It not what the code actually testing. So it don't really break any test, and should not be consider a breaking change.
But yeah, I agree with your assessment how this change happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chigia001 did the meaning of the test here change? This indicates that a link should now be treated as a page and not a link? The previous one seemed like it dealt with something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pailhead I'm not going to defend this specific change. I just want to anwser the reviewer question on How did the tests work before?
, and why that should not consider a Breaking Change.
This change do indicate the method of how the PR's owner remove the code, (which is sensible approach, I will do the same if I have similar requirement), and lack of code-review from the approver.
@@ -569,7 +569,7 @@ test("test before-cache event", async ({ page }) => { | |||
assert.equal(await page.textContent("body"), "Modified") | |||
}) | |||
|
|||
test("test mutation record as before-cache notification", async ({ page }) => { | |||
test("test mutation record-cache notification", async ({ page }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to have been removed by error like mentioned in my previous comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is testing the mutation record as it tested it before, with this additional notification caching. It’s probably an intended change just doesn’t belong in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change and the other similar one seem to be due to a global find-replace to remove as X
, which is a TypeScript syntax. kind of funny it affected these test descriptions
You do you, but this breaks about every PR in the repo (including 3 from me for which I haven't received any feedback) without any prior notice. I would lie if I said I'm surprised, but this is certainly a slap in the face for anybody who likes to contribute. Certainly a lesson to me to not rely on consistent open source governance from you. |
🤡 |
This is a nice example of how to kill contributions from other people in 3.. 2.. 1.. |
This is a fine example of a phillosophycal PR. Great work making your point! |
This looks like political activism more than a thought through decision. |
As someone who has benefitted tremendously from the opinionated decision making that produced active record, rails, turbo, etc... I'll just say thanks for being decisive. I'd rather deal with change than indecision and bloat. Also, TypeScript hurts to write. Good riddance. 😄 |
Absolute tomfoolery. |
Folks considering removing typescript in their codebases should have at the least JSDocs as alternative as this is just hostile to library users and contributors relying on IDE autocomplete. |
It's funny suggestion consider how many people commited on project =) |
Since going full TS last year with work, personal projects, and freelancing, my usage of profanity/curse words has increased 💯. Most days (even today) I just curse and swear and want to cry. Most people here hate and suffer from TS more than they care to admit, but whatever 😴 Great decision! |
I’m an Open Source contributor myself and I know how hard it is to contribute. A change like this would really grind my gears. Especially if I have a PR open where I contributed my valuable time. Edit: yes this was a quote of myself, it should have been part of my original comment |
I love internet ❤ 🤣😁 |
If you want to see someone justify their decision to remove typescript without dismissing it as a holy war, go read Rich Harris' explanation on why Svelte moved from TypeScript to JS+JSDoc. Real solidly reasoned opinion that not everyone will agree with, but it truly points out some of the issues with TypeScript and why a maintainer MIGHT want to move away from it. Rich Harris:
The disrespect of not preparing your PR submitters for this is also awful. |
this sucks a lot! |
Let me just say I'm learning a lot about OSS from these "conversations" |
turbo is now RIP. removing end-user interfaces without any alternatives is bold decision, but also useless |
What we're afraid of is not TS vs. JS. It is the lack of listening and diversity of inputs of the Rails community. |
A significant portion of this PR contains functional changes to modify public 'private-type' fields to be actually private. Is this intended? These are functional changes that will affect anyone who relied on these fields being technically public. Personally, I am all for the change to use the proper private field syntax. |
Lol... let's see if he'll lose an arm because you're not using Turbo. |
The thing is, it's not about whether javascript or typescript is more suitable for this library. It's about how OSS should operate. Rushing a PR that, not just only have breaking changes but change the language of the library altogether is big no-no. |
I can certainly support the swift eradication of TypeScript, but this is certainly not the way to go about it. |
Of course as a rational person I agree with pursuing speed and efficiency at all costs. Personally, I don't think removing TypeScript goes far enough. This library should be immediately rewritten in Rust as that is the only way to achieve maximum efficiency in both development and runtime speed. See CompVis/stable-diffusion#283 for an example of a similar suggestion that was wildly successful. Unfortunately this will require adding static typing back to the library, but it will certainly be worth it in the end. |
Skill issue |
What are you talking about? HTMX don't use TS either. They only have one big |
Something is wrong with your eyes. Reads much better? Bro youre not thinking clearly, thats for sure. Reconsider. Clearly this is a terrible decision. |
As the vice mayor of flavortown, I can tell you that there is no seasoning or spice to make dhh's boots taste better. |
It is a governance thing, not an 'I am afraid of typescript' thing. You make choices with dependencies and how dhh operates now and historically makes following the rails way fraught with churn and irrational + emotional decisions. I trust htmx or unpoly governance far far more than dhh. |
Yesterday, I was in the process of refactoring our Rails project and was considering whether to introduce TypeScript. Coincidentally, I saw this perspective today. I respect the decision; in fact, our Rails project has a lot of JavaScript (React), and we have other projects that use next.js and TypeScript. Writing TypeScript and dealing with endless build errors do pose challenges to my productivity, but I believe this represents the quality of software delivery. This doesn't mean the software we deliver using JavaScript is of lesser quality, but it's an opportunity for me to seriously study and learn how to do this right. |
https://twitter.com/taylorotwell/status/1699535502054170837?s=20 I think all the people who are arguing hard for this are the ones being disrespectful, feel free to fork and move on. |
The funny thing is HTMX is the prime example of pure JavaScript joy: a single unreadable file that just God and the browser needs to care about. Never used turbo, but any change from .ts to .js is a great change, because now no one can has the option of pretending that TypeScript is any good. |
Agreed! 🥱 |
A PR with a lifespan of only 2 hours, covering ~100 files, stripping types everywhere, changing some properties to private fields or getters, adding entirely new files... this all seems perfectly thought through. |
Maybe he spent exactly 2 hours reviewing it, how do you know? :) How long do you need to review a PR? Days? Obviously they prepared, wrote a blog post etc, he knew lots about it before a PR popped up lol |
Nobody is talking about the removal of Prettier in this PR. Now we can get back to the glorious spirt of writing code in a way that matches our personal aesthetics, free from the dogmatic opinions of code formatters! |
Atleast political activism goes toward some sort of goal. This is just stupidity. |
I love TypeScript but I kinda see your point in converting to JS. Still, there should've been a heads up. For both your end users and contributers, because at the end of the day they are your audience. Your end users. Changing the entire codebase without ANY notice is absolutely preposterous. To everyone saying "good riddance!", imagine spending hours of your time, making changes to this repo, only for the author to say "we going to JavaScript, meaning all your PRs are donezo!" It's disrespectful to everyone involved. |
https://world.hey.com/dhh/turbo-8-is-dropping-typescript-70165c01