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

Remove TypeScript #971

Merged
merged 8 commits into from Sep 6, 2023
Merged

Remove TypeScript #971

merged 8 commits into from Sep 6, 2023

Conversation

afcapel
Copy link
Collaborator

@afcapel afcapel commented Sep 6, 2023

@marcoroth
Copy link
Member

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.

@adrienpoly
Copy link
Member

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 :

  • lot's of the Hotwire eco system packages will be broken
  • All current open PRs are now fully obsolete. Some from my point of view were very good candidates
  • IDE won't provide any more autocompletion as they did

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

@rik
Copy link
Contributor

rik commented Sep 6, 2023

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)

@seanpdoyle
Copy link
Contributor

seanpdoyle commented Sep 6, 2023

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 interface that maps to callbacks that accept arguments only really works with some kind of type enforcement. Without it, interfaces and delegates are prone to drift apart.

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?

@dhh
Copy link
Member

dhh commented Sep 6, 2023

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.

@dhh dhh merged commit 9f3aad7 into main Sep 6, 2023
2 checks passed
@dhh dhh deleted the untyped branch September 6, 2023 13:58
Copy link

@vfonic vfonic left a 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",
Copy link

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

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?

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.

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

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

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

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.

Copy link

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?

Copy link

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).

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

Copy link

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.

Copy link

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

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

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

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"

Copy link

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?

Copy link

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 🤷‍♂️?

Copy link

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.

Copy link

@pailhead pailhead Sep 6, 2023

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.

Copy link

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

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)"

Copy link

@chigia001 chigia001 Sep 6, 2023

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.

Copy link

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.

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 }) => {
Copy link

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

Copy link

@pailhead pailhead Sep 6, 2023

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?

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

@julianrubisch
Copy link
Contributor

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.

@hanayashiki
Copy link

🤡

@JoshuaLuckers
Copy link

This is a nice example of how to kill contributions from other people in 3.. 2.. 1..

@gassius
Copy link

gassius commented Sep 6, 2023

This is a fine example of a phillosophycal PR. Great work making your point!

@anri-asaturov
Copy link

This looks like political activism more than a thought through decision.

@jhubert
Copy link

jhubert commented Sep 6, 2023

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. 😄

@NSAntoine
Copy link

Absolute tomfoolery.

@cvpcasada
Copy link

cvpcasada commented Sep 6, 2023

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.

@ReDBrother
Copy link

ReDBrother commented Sep 6, 2023

This is a nice example of how to kill contributions from other people in 3.. 2.. 1..

It's funny suggestion consider how many people commited on project =)
I think that changes it's huge releave for core developers)

@franklin-tina
Copy link

Since going full TS last year with work, personal projects, and freelancing, my usage of profanity/curse words has increased 💯.
Some days, it feels like I know TS.
Things would work as expected.
Good days.

Most days (even today) I just curse and swear and want to cry.
Then I sign off with 'any' or bloody casting.

Most people here hate and suffer from TS more than they care to admit, but whatever 😴

Great decision!

@JoshuaLuckers
Copy link

JoshuaLuckers commented Sep 6, 2023

This is a nice example of how to kill contributions from other people in 3.. 2.. 1..

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

@junaidveeve
Copy link

I love internet ❤ 🤣😁

@AndKenneth
Copy link

AndKenneth commented Sep 6, 2023

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:

Lordy, I did not expect an internal refactoring PR to end up #1 on Hacker News. Let me provide some context, since a lot of people make a lot of assumptions whenever this stuff comes up!
If you're rabidly anti-TypeScript and think that us doing this vindicates your position, I'm about to disappoint you. If you're rabidly pro-TypeScript and think we're a bunch of luddite numpties, I'm about to disappoint you as well.

Firstly: we are not abandoning type safety or anything daft like that — we're just moving type declarations from .ts files to .js files with JSDoc annotations. As a user of Svelte, this won't affect your ability to use TypeScript with Svelte at all — functions exported from Svelte will still have all the same benefits of TypeScript that you're used to (typechecking, intellisense, inline documentation etc). Our commitment to TypeScript is stronger than ever (for an example of this, see https://svelte.dev/blog/zero-config-type-safety).

I would say that this will result in no changes that are observable to users of the framework, but that's not quite true — it will result in smaller packages (no need to ship giant sourcemaps etc), and you'll be able to e.g. debug the framework by cmd-clicking on functions you import from svelte and its subpackages (instead of taking you to an unhelpful type declaration, it will take you to the actual source, which you'll be able to edit right inside node_modules to see changes happen). I expect this to lower the bar to contributing to the framework quite substantially, since you'll no longer need to a) figure out how to link the repo, b) run our build process in watch mode, and c) understand the mapping between source and dist code in order to see changes.

So this will ultimately benefit our users and contributors. But it will also benefit us, since we're often testing changes to the source code against sandbox projects, and this workflow is drastically nicer than dealing with build steps. We also eliminate an entire class of annoying papercuts that will be familiar to anyone who has worked with the uneven landscape of TypeScript tooling. The downside is that writing types in JSDoc isn't quite as nice as writing in TypeScript. It's a relatively small price to pay (though opinions on this do differ among the team - this is a regular source of lively debate).

We're doing this for practical reasons, not ideological ones — we've been building SvelteKit (as opposed to Svelte) this way for a long time and it's been miraculous for productivity.

The disrespect of not preparing your PR submitters for this is also awful.

Sauce: https://news.ycombinator.com/item?id=35892250

@aymende7
Copy link

aymende7 commented Sep 6, 2023

image

@sdcoffey
Copy link

sdcoffey commented Sep 6, 2023

this sucks a lot!

@AndKenneth
Copy link

Matt put it best
image

@Boye95
Copy link

Boye95 commented Sep 6, 2023

Let me just say I'm learning a lot about OSS from these "conversations"

@pi0
Copy link

pi0 commented Sep 6, 2023

IMG_3954

@iwdt
Copy link

iwdt commented Sep 6, 2023

turbo is now RIP. removing end-user interfaces without any alternatives is bold decision, but also useless

@dahchon
Copy link

dahchon commented Sep 6, 2023

What we're afraid of is not TS vs. JS.

It is the lack of listening and diversity of inputs of the Rails community.

@eligrey
Copy link

eligrey commented Sep 6, 2023

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.

@franklin-tina
Copy link

Matt put it best image

Matt is one of the nicest people on X, but he also sells TS courses/workshops.
I wouldn't expect him to agree.

Lol, like you're expecting NRA executives to support laws banning guns 😂

@franklin-tina
Copy link

Solidified my reasoning to use htmx over turbo. Your governance has been a shortlist of how not to treat a project and community, thanks 👍🏻

Lol... let's see if he'll lose an arm because you're not using Turbo.

@iiAbady
Copy link
Contributor

iiAbady commented Sep 7, 2023

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.

@yellowberryHN
Copy link

I can certainly support the swift eradication of TypeScript, but this is certainly not the way to go about it.

@phinnaeus
Copy link

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.

@adeleke5140
Copy link

Most days (even today) I just curse and swear and want to cry. Then I sign off with 'any' or bloody casting.
Most people here hate and suffer from TS more than they care to admit, but whatever 😴

This is a sign you keep writing javascript with types instead of writing typescript. Gotta change the approach and habits. Most things causing pain are just terrible attempts to write dYnAmiC code.

TS still sucks! 🤷🏿‍♂️

Skill issue

@kg-currenxie
Copy link

Solidified my reasoning to use htmx over turbo. Your governance has been a shortlist of how not to treat a project and community, thanks 👍🏻

What are you talking about? HTMX don't use TS either. They only have one big .d.ts definitions file, which if you need, can contribute yourself over in the @types community github organization (DefinitelyTyped) like thousands of other libraries who didn't wanna port their whole codebase from .js

@0nk3
Copy link

0nk3 commented Sep 7, 2023

https://world.hey.com/dhh/turbo-8-is-dropping-typescript-70165c01

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.

Something is wrong with your eyes. Reads much better? Bro youre not thinking clearly, thats for sure. Reconsider. Clearly this is a terrible decision.

@mmm-zesty
Copy link

Solidified my reasoning to use htmx over turbo. Your governance has been a shortlist of how not to treat a project and community, thanks 👍🏻

Lol... let's see if he'll lose an arm because you're not using Turbo.

As the vice mayor of flavortown, I can tell you that there is no seasoning or spice to make dhh's boots taste better.

@mmm-zesty
Copy link

Solidified my reasoning to use htmx over turbo. Your governance has been a shortlist of how not to treat a project and community, thanks 👍🏻

What are you talking about? HTMX don't use TS either. They only have one big .d.ts definitions file, which if you need, can contribute yourself over in the @types community github organization (DefinitelyTyped) like thousands of other libraries who didn't wanna port their whole codebase from .js

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.

@linyiru
Copy link

linyiru commented Sep 7, 2023

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.

@Esteban-Rocha
Copy link

image

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.

@BreytMN
Copy link

BreytMN commented Sep 7, 2023

Solidified my reasoning to use htmx over turbo. Your governance has been a shortlist of how not to treat a project and community, thanks 👍🏻

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.

@franklin-tina
Copy link

Most days (even today) I just curse and swear and want to cry. Then I sign off with 'any' or bloody casting.
Most people here hate and suffer from TS more than they care to admit, but whatever 😴

This is a sign you keep writing javascript with types instead of writing typescript. Gotta change the approach and habits. Most things causing pain are just terrible attempts to write dYnAmiC code.

TS still sucks! 🤷🏿‍♂️

Skill issue

Agreed! 🥱

@chrissnyder chrissnyder mentioned this pull request Sep 7, 2023
@jpike88
Copy link

jpike88 commented Sep 7, 2023

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.

@kg-currenxie
Copy link

kg-currenxie commented Sep 7, 2023

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

@riley-worthington
Copy link

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!

@shimeji87
Copy link

shimeji87 commented Sep 7, 2023

This looks like political activism more than a thought through decision.

Atleast political activism goes toward some sort of goal. This is just stupidity.

@TechStudent10
Copy link

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.

@hotwired hotwired locked as too heated and limited conversation to collaborators Sep 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet