Event.desc type is still a string

[YizYah]

This won't work:

if (event.desc === holidayDesc.LAG_BAOMER)

because the event.desc type is string rather than holidayDesc.

[mjradwin]

Surprised to hear that, but I am not a TS expert so I believe you. Perhaps we can change the type definition from an enum to const object with properties that are strings?

[mjradwin]

OK, try 5.3.12 and let us know if it works for you now

[YizYah]

@mjradwin unfortunately, our time zones are almost mirror opposite, so it is hard to have timely feedback.

PLEASE restore holidayDesc to being an enum. Much better. Should really be called HolidayDesc, because the enum is used as a type, and types normally use PascalCase.

The problem is the specification of the Event class. I have never generated type files by hand, but it seems to me that you need these changes:

getDesc(): HolidayDesc;
...
readonly desc: HolidayDesc;

I'll see if I have some time in the next day or two, although things are a bit tight. It would seem that I'm the user that wants this the most LOL.

[mjradwin]

Wouldn't changing the type signature from string to the HolidayDesc enum be a breaking change and require the major version number for @hebcal/core to increase from 5 to 6?

What's wrong with the solution we implemented with a const object exporting strings? Does that not work correctly with your app?

[YizYah]

Hmmm, I guess it would be breaking technically. I say that because I doubt that many users would have faced a problem.

It does work. But as far as why an enum is a better solution IMO:

1. there's more type safety. It's a new type. You can't assign any value to a variable of an enum type that isn't a member of that enum. In this case, the desc is really a name for an event in practice. In effect, it's an identifier on my end. So I'd prefer to have it typed strongly.
2. we'll get autocompletion for enum values. Strongly typed code practically writes itself today!

I do wonder how many users would have to update anything. But, you're point is taken!

[mjradwin]

The other reason to make it a string is that there can be arbitrary subclasses of Event who have a description that isn't in the HolidayDesc enum.

For 30+ years Hebcal has displayed the first day of Rosh Hashana with the Hebrew year, for example "Rosh Hashana 5784". In theory we could change the Event.desc to be "Rosh Hashana I" to be consistent with the naming convention for multi-day holidays and have the version with the year displayed by Event.render() - but this would require some additional work and more unit testing.

Similarly, for MoladEvent, the current description is something like "Molad Kislev 5784" containing the name of the month and year.

We would also, for example, need to add dozens of extra items to the enum for every Rosh Chodesh, Shabbat Mevarchim Chodesh, and Yom Kippur Katan, as each of these events has the name of the month in the description.

All of that extra work introduces risk, plus the type signature change/breakage, makes it feel like too big of a stretch

[YizYah]

I see. So, if you are adding things to the name of the holiday, such as a year, then it really is a description, not a name. If so, indeed it should stay a string.

But, maybe we could both be 100% happy here... I am inclined to say that we could add an additional field to Event with an enum that identifies the holiday. For instance, HolidayName.RoshHaShana. So we could have Event.holidayName be typed as HolidayName|undefined and then downstream anyone could use it with full control.

I think that it should actually be called HolidayName rather than HolidayDesc, because desc is not the same thing.

The benefit of having Event.holidayName is that then I could have something like

const holidays: Record<HolidayName, HolidayInfo> = {
[HolidayName.RoshHaShana]: { ...}
...
}

Then, if in the future you add Behab to the list of holidays, I'll immediately know when I upgrade because I'll get a static error that Behab is missing. The holidays Record will be defined for every holiday, which simplifies code and keeps me up-to-date.

Then we could just leave Event.desc untouched. The only thing is that when you derive "Rosh Hashana 5784" you would ideally use the HolidayName, so that there would be no duplication of data.

[mjradwin]

In theory, the Event flags are supposed to serve this purpose. They aren't the best API design. They were ported directly from the original Hebcal C implementation, and JS purists understandably might object to bitmasking. A couple of years later, we introduced an event "categories" concept to compensate for this so users of the API would be able to deal with short stable strings like roshchodesh.

Because we already have all 3 of description, flags, and categories, I really I prefer to avoid adding another enum to Event.

I might be open to an enum based solution if we wanted to make the more radical API breaking change to eliminate the flags and categories. This complete redesign would be safer to introduce after we had already completed a JS => TS rewrite and had strong guarantees of type safety.

[YizYah]

Yes, that does make sense.