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

Refactor to work with Tree Shaking #433

Closed
SLaks opened this issue Oct 7, 2024 · 3 comments
Closed

Refactor to work with Tree Shaking #433

SLaks opened this issue Oct 7, 2024 · 3 comments

Comments

@SLaks
Copy link
Contributor

SLaks commented Oct 7, 2024

Motivation

I wrote some very simple code to run in a browser and print a full year of פרשה names:

import { HebrewCalendar } from '@hebcal/core'
const events = HebrewCalendar.calendar({
  year: new Date().getFullYear() - 1,
  numYears: 2,
  sedrot: true,
  noHolidays: true,
})

console.log(
  events.map((event) => ({
    datetime: event.date.greg(),
    label: event.render('he-x-NoNikud'),
  }))
)

When compiled for production using https://vitejs.dev/ with default settings, this produced 150 kB of optimized JS!

dist/index-Cca6IFY6.js                    150.13 kB │ gzip: 53.69 kB

Since all functions are contained in the HebrewCalendar class and the Event hierarchy, Tree Shaking cannot see which APIs are actually used.

Suggestion

It ought to be possible to build a new modular API in which application code only imports features that are actually used. This should allow tree shaking to entirely drop unused features (eg, emojis, zmanim calculations, unused locales, etc).

This would be a new API that requires explicit imports to enable more calculations and methods. The existing API would remain as a wrapper to avoid breaking existing code. Applications that want to benefit from tree shaking would need to migrate to the new API to gain the benefits.

Details

Making code tree-shakable involves a number of changes (these changes can be made independently for incremental improvements, but each incremental change would require clients to refactor further to get more benefit):

Note: All of these suggestions are rough ideas; I have not reviewed the code carefully enough to be sure that each of these make sense and would actually allow non-trivial amounts of code to be tree-shaken

  • Replace the static methods on HebrewCalendar with separate exports to drop unused methods.
    • This is the simplest improvement, and should be able to drop files like tachanun.ts and hallel.ts easily.
  • Replace locale name strings with separately-exported locale objects containing their translations.
    • This way, if a project only uses one locale, other locales will be dropped.
    • Unless you expect projects to use both he and he-x-nonikud, it would also make sense to move he-x-nonikud to a separate file generated at build time, so that projects that only use it can drop nikud entirely.
  • Replace the flags enum with an array of importable objects that contain their Event subclasses and the relevant parts of getHolidaysForYear_()
    • This way, projects that only use certain flags can drop all code for other events.
    • Fully implementing this would involve a major refactor of getHolidaysForYear_(); we should try to estimate how much code this could actually save first.
  • Similarly to the previous bullet, replace the various boolean options in HebrewCalendar.calendar() with an array of imported functions or plugins to specify which events you want returned.
    • This would let projects drop all of the Zmanim code (if they don't pass candleLighting or omer), which should be a very big win.
    • This can also improve end-user type safety; you can declare the function as returning an array of Event subclasses based on the plugins you pass.
    • This can be implemented in stages; declare all of the plugins first (so that consumers can import only what they need), and you can then slowly split apart the underlying code to be more modular. This allows you to make incremental improvements under the hood without forcing application code to change further.
    • Similarly, you could eventually split apart getHolidaysForYear_() based on plugins as well for further savings.
  • Move any expensive members of Event and subclasses (eg, getEmoji(); I don't know if there are any other costly members) to a top-level exported function that takes the event as a parameter instead.
    • To be less disruptive, you could declare an exported function that adds getEmoji() to Event.prototype, so that it is still available as a member function, but only if this is imported.
    • This can even maintain full type safety in TypeScript; example (TypeScript will only see this file, and therefore the prototype declaration, if imported).
    • I haven't tried it, but you can probably make this even safer in TypeScript by declaring the importable installer function as asserts Event is {prototype: {getEmoji(): string}}

Let me know if you need help understanding, designing, prototyping, or implementing this.

@mjradwin
Copy link
Member

mjradwin commented Oct 7, 2024

Great suggestion! @hebcal/core was definitely not designed with minimal code-size as a goal. For example, there are some parts of the library such as Hallel and Tachanun that are rarely used, and could easily be separate libraries. The zmanim functionality is used much more widely (but not by every application), and it pulls in a large dependency on an elevation-aware sunset calculator - it could be split out, but it would be significant work to do so.

Unfortunately, I don't have the time to undertake such a large project -- I just started a new job and it may be many months before I could try to tackle this.

I'm totally supportive of this kind of work if it can be done without breaking compatibility. Is this something you want to attempt?

There was an earlier attempt to reduce the size of generated code with some modest success. As I was learning TypeScript, I split out HDate into its own package @hebcal/hdate which does Hebrew-Gregorian date conversions but has no notion of holidays or parsha.

To preserve backwards compatibility and to avoid breaking clients of @hebcal/core, the entire @hebcal/hdate library is bundled into @hebcal/core.

@SLaks
Copy link
Contributor Author

SLaks commented Oct 7, 2024

I'm also busy with other stuff right now; I will probably take a stab at this in a month or two.

I would probably do expose this as multiple files in a dedicated subfolder, so that application code would look like import {includeModernHolidays} from '@hebcal/core/modular/inclusions'

@SLaks
Copy link
Contributor Author

SLaks commented Oct 31, 2024

Moved to hebcal/hebcal-es6#475, because I filed this in the wrong repo

@SLaks SLaks closed this as not planned Won't fix, can't repro, duplicate, stale Oct 31, 2024
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

No branches or pull requests

2 participants