hebcal / hebcal-es6

perpetual Jewish Calendar with holidays, Shabbat and holiday candle lighting and havdalah times, Torah readings, and more
https://hebcal.github.io/api/core/
GNU General Public License v2.0
98 stars 14 forks source link

Starting to Introduce TS #442

Closed YizYah closed 3 months ago

YizYah commented 3 months ago

I just want to make a general point here about TS. You can introduce it in tiny steps. You should not have to remove js files just because you start to add some TS files. That said, because you are currently generating hebcal.d.ts manually, getting started would be harder than normal. I actually tried it yesterday for about 45 minutes, and I was having some problems.

The thing is, currently you have two specifications of the same enums (not to mention the classes), which seems very brittle to me. The first issue that I reported was that there were members for Zmanim that I saw clearly documented, but that were not showing up. I am guessing that the problem was that they were not updated in both places.

In addition, TS would make it far simpler for changes to the code base. I suspect that 95% of the errors that could be caught by the tests would be caught statically by TS. For instance, I think I accidentally left off a field in one of the staticHoliday elements when I introduced the hebrewDesc enum. Right now, there's no protection from that at all. A simple typo for a key could result in something breaking.

Also, working with the code becomes far simpler. For any new staticHoliday, vs code should be popping up the neeeded fields (and CoPilot should be filling them in!). And, cascading incremental updates to types becomes trivial.

When it comes to config, I am no expert. It may be helpful to have someone advise on how to replace hebcal.d.ts incrementally. I'm inclined to say that we should introduce TS into the package as if the hebcal.d.ts file does not exist, and add to the build script a temporary bash-level fix, such as copying the hebcal.d.ts file into dist and/or appending its contents to another file where needed. That way, we could replace the contents one step at a time.

I also think it would be good to talk live if you would be open to the idea.

mjradwin commented 3 months ago

Would you like to meet up on Sunday evening Israel time, maybe 17:00 or 18:00? This is a good time for me as the kids don't have anywhere to be on Sunday mornings here in California.

I am in general agreement that long term maintenance is easier with TypeScript and if I had the free time I would be willing to undertake a complete rewrite.

We have done a tiny bit of work on TS rewrite by moving some of the code from @hebcal/core into @hebcal/hdate, and perhaps the collection of smaller packages is one approach to doing the refactor in pieces without introducing the risk of making all of the changes all at once. For example, a hypothetical @hebcal/holidays package could just have all-day untimed holiday events, but none of the complexity of zmanim. Another package might have zmanim only.

The @hebcal/core package could then concatenate the generated typescript defs using a bash script as you describe.

YizYah commented 3 months ago

@mjradwin check out https://github.com/hebcal/hebcal-es6/pull/444. I would argue against modularizing. There isn't much need, because until you have @hebcal/core itself converted at least for the index file (as I do there), you haven't achieved much, and the bash command could be messy. And, part of the reason why mono-repos are becoming so popular is the strong typing allowed by TS. So, once it's TS, you will lose out a lot by having changes not propagating. Honestly, I think you should consider restoring hdate after the core is converted.

I really don't think it should be hard to move it one file at a time, top down. I've already done index.ts for you.

I would enjoy talking, not sure I'll have time this Sunday. If I think I do, I'll reach out in email. If not, maybe a week from Sunday. I'll send an invite bli neder and you can reply.

mjradwin commented 3 months ago

Thanks for your changes! I accepted the pull request and merged it in.

I started taking a look at porting Locale from JS to TS yesterday - I'll see if I can introduce that change to build on top of your work.

YizYah commented 3 months ago

Thanks! Your follow-through is exemplary @mjradwin.