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

[FEEDBACK WANTED] Proposal for 1.0.0 breaking changes #1039

Open
gilmoreorless opened this issue Feb 26, 2023 · 8 comments
Open

[FEEDBACK WANTED] Proposal for 1.0.0 breaking changes #1039

gilmoreorless opened this issue Feb 26, 2023 · 8 comments
Labels
Milestone

Comments

@gilmoreorless
Copy link
Member

Hi everyone. I've recently (last year) joined the maintenance team for Moment Timezone, and I'd like to gather feedback on some proposed changes for the project. First I should clarify the current status. As stated in the primary documentation:

We now generally consider Moment to be a legacy project in maintenance mode. It is not dead, but it is indeed done.

In practice, this means:

  • We will not be adding new features or capabilities.
  • We will not be changing Moment's API to be immutable.
  • We will not be addressing tree shaking or bundle size issues.
  • We will not be making any major changes (no version 3).
  • We may choose to not fix bugs or behavioral quirks, especially if they are long-standing known issues.

However, since we understand that Moment is well established in millions of existing projects:

  • We will address critical security concerns as they arise.
  • We will release data updates for Moment-Timezone following IANA time zone database releases.

That said, I've been slowly going through the backlog of open issues for Moment Timezone and I've noticed some common trends:

  1. The way that moment-timezone requires and modifies core moment causes a lot of confusion and problems with dependency management.
  2. The time zone data files try to cover too many use cases at once. But there's no one single solution that will satisfy everyone's needs. (e.g. Dramatic increase in package size #999 (comment))
  3. The expectations of 0.x.y semantic version numbers are not well understood, and don't make sense for such a long-running project.

While keeping in mind the guidelines listed above, there's no way to fix these common problems without a breaking change.

Therefore, I'm proposing a ⚠️ one-time breaking change release to move moment-timezone to v1.0.0 ⚠️.

There are 5 potential changes that I'd like feedback on, but 3 of them are closely related so they're listed as "a", "b" and "c" variations. Each proposed change will be in a separate comment, to allow separate inline reactions (such as 👍 or 👎) on each one. If you have more detailed feedback, feel free to leave a constructive comment.

But what about "maintenance mode"?

As noted above, the Project Status section of the Moment documentation says that major changes like this will not be happening. So isn't this plan a direct contradiction of that public statement? Sort of.

The documented project status is mostly focused on the core Moment library, which is certainly more widely-used than the Moment Timezone extension. The line "We will not be making any major changes (no version 3)" is targeted at work that had been started to completely revamp Moment's internals and move to an immutable API. Moment Timezone has some slightly looser restrictions:

  1. We still need to do occasional releases for Moment Timezone to keep up with IANA tzdb changes. Therefore there's a necessity to avoid creating more problems in dependent projects in future.
  2. These proposed changes are mostly about making maintenance of the project easier. A small-scale rework in the short term which reduces maintenance hassles in the long term.
  3. The changes have a fairly small surface area, and don't involve rewriting large chunks of the codebase.

Ultimately, this issue is not a guarantee of change, it's a call for feedback. If there's a big pushback to some or all of the proposals, they won't be implemented. There's also no defined timeline yet, just some ideas. There's no rush on this project (I actually only agreed to join the team because the project is in maintenance mode — I get to things in my own time).

@gilmoreorless
Copy link
Member Author

1. Move moment from dependencies to peerDependencies

What this means

When Moment Timezone is installed, it currently includes a version of core Moment as a dependency. In most cases this isn't a problem, but due to dependency updates and automated tools such as Dependabot, there can sometimes be multiple versions of core Moment in a project. This then causes problems when require('moment') and require('moment-timezone') return 2 different instances of Moment.

Moving moment to peerDependencies means that Moment Timezone will no longer include its own Moment dependency, and it will be up to the parent project to provide it.

This change was previously done in Moment Timezone 0.5.18 before it was known that it would break some projects. It was quickly reverted in 0.5.19 (as mentioned in #799).

Consequences

  • Projects that aren't using Node.js and package.json won't be affected. For example, if you're using one of the pre-built files like <script src="moment-timezone-with-data-10-year-range.js"> directly in a browser.
  • Projects that are using Node.js, but are requiring one of the pre-built files won't be affected. e.g. require('moment-timezone/builds/moment-timezone-with-data-10-year-range.js').
  • Projects that define both moment and moment-timezone in their package.json dependencies shouldn't be affected. When Moment Timezone calls require('moment') it will find the parent project's existing moment dependency.
  • Projects that define only moment-timezone in their package.json will need to add moment as a core dependency.

@gilmoreorless
Copy link
Member Author

2. Remove the pre-built moment-timezone-with-data-2012-2022 files

What this means

This one is fairly straightforward. The "2012-2022" files started off as "2010-2020" builds, defined initially as "current year ± 5 years". When they were updated in 2017 to be "2012-2022", there were complaints about broken projects because the file names had changed.

To solve this, new moment-timezone-with-data-10-year-range files were created to cover the rolling "± 5 years" period. The "2012-2022" files were kept only for backwards compatibility (see #614 (comment) for more details).

But it's now 2023, so those builds are obsolete and are often giving the wrong data for current dates. A deprecation warning was added to the files in version 0.5.41. A 1.0.0 release is the best time to remove these files completely.

Consequences

  • Projects importing moment-timezone-with-data-2012-2022.js or moment-timezone-with-data-2012-2022.min.js from the npm package (or via a service like CDNJS) will need to change to a different pre-built data file.
  • This won't affect direct-download files on momentjs.com, as the "2012-2022" files were removed back in 2019.
  • The other pre-built files such as 10-year-range and 1970-2030 will remain unchanged.

@gilmoreorless
Copy link
Member Author

gilmoreorless commented Feb 26, 2023

3a. Change the packed data format

What this means

The packed data format used by Moment Timezone is an efficient text-based encoding of the tzdb data. For the full tzdb dataset, a 12MB unpacked/latest.json gets shrunk to ~900KB packed/latest.json.

But it's not as efficient as it could be, which has been exposed by recent complaints about the package size (#999). The maximum efficiency would be to use a custom binary format instead of JSON, but that's unfeasible for several reasons. There are some significant gains that can be made within the current format, though — mostly by reducing a lot of repetition.

The packed JSON format has changed a few times before, but they've always been additive changes that are backwards-compatible with older versions. These proposed updates aren't a massive revamp of the format, but will still change the data in a way that's incompatible with parsing in older versions.

Consequences

  • For the huge majority of use cases, nothing will change, as the data files and loading code are bundled together.
  • Packed data files from 0.x.y releases of Moment Timezone can be loaded by 1.0.0+.
  • Packed data files from 1.0.0+ releases will break if loaded by an earlier version of Moment Timezone.
  • Any project that parses the packed JSON data separately will need to update to handle the new format.

Examples

The exact format changes aren't specifically defined yet, as I've only done some small experiments. However, these would be the most likely changes:

Expand example snippets
  1. countries: Consecutive identical region names in lists can be truncated.

    - "AQ|Antarctica/Casey Antarctica/Davis Antarctica/Mawson Antarctica/Palmer Antarctica/Rothera Antarctica/Troll Asia/Urumqi Pacific/Auckland Pacific/Port_Moresby Asia/Riyadh Antarctica/McMurdo Antarctica/DumontDUrville Antarctica/Syowa Antarctica/Vostok",
    + "AQ|Antarctica/Casey /Davis /Mawson /Palmer /Rothera /Troll Asia/Urumqi Pacific/Auckland /Port_Moresby Asia/Riyadh Antarctica/McMurdo /DumontDUrville /Syowa /Vostok",
  2. links: Multiple links to the same target zone can be combined, then truncated like countries.

    - "America/Panama|America/Atikokan",
    - "America/Panama|America/Cayman",
    - "America/Panama|America/Coral_Harbour",
    + "America/Panama|/Atikokan /Cayman /Coral_Harbour",
  3. zones: Repeating patterns of numbers can be represented using a form of run-length encoding.

- "Africa/Tunis|LMT PMT CET CEST|-E.I -9.l -10 -20|01232323232323232323232323232323232|...",
+ "Africa/Tunis|LMT PMT CET CEST|-E.I -9.l -10 -20|01{23,g}2|...",

The best result I've got so far is 22% of the original JSON size (805KB -> 183KB) without any loss of data, which is even more efficient than using a generic tool such as msgpack.

@gilmoreorless
Copy link
Member Author

3b. Restrict the maximum future data range

What this means

This is related to the data size problems detailed at #999 (comment). The main reason the data files are so large lately is that we're using the full 64-bit versions of the tzdb's zic and zdump utilities. These produce time zone data up to the year 2500.

However, the chances are very low that the predicted time zone data is still accurate that far in the future. World governments change their time zone rules frequently, often at short notice, which is why there are several tzdb releases every year.

I propose restricting the full packaged time zone data to 100 years in the future, based on the year of release for a Moment Timezone version. All past historical data would still be included, as that is (mostly) more accurate and far less likely to change. So for example, a package release built in 2023 would include data from 1844 to 2123. I feel this is a reasonable trade-off between data accuracy and small(ish) file sizes. (The packed JSON file would drop to less than half its current size, even without compression.)

This could be done in addition to compressing the data format as described above, or instead of it, depending on feedback.

Consequences

  • It would not be possible to get correct DST-aware timestamps for dates more than 100 years in the future. But since a lot of goverments are currently investigating removing daylight saving anyway, there's no guarantee that the currently-published data is correct either, even 5 years in the future.
  • This only affects users who are using the full moment-timezone-with-data files, or directly importing/requiring the library using Node.js.
  • The other pre-built files such as 10-year-range and 1970-2030 will continue to be packaged as usual. People using those files won't be affected by this change.

@gilmoreorless
Copy link
Member Author

3c. Extract data updates into separate packages

What this means

Currently the moment-timezone npm package includes the calculation code and the time zone data together. Most Moment Timezone version bumps are only due to the IANA tzdb having a new release, with no functionality changes. Instead of trying to handle everyone's data use cases in the same package, we could try splitting them apart.

In basic terms:

  • moment-timezone becomes a code-only package.
  • New separate data packages are released to cover different ranges (e.g. moment-timezone-data-10-year or moment-timezone-data-full, or perhaps scoped packages like @momentjs/tzdata-full).
  • A simple integration method is created to allow loading the data package of your choice.

Honestly, I haven't fully explored all the details of this option. There are a large number of unknown questions and consequences — e.g. how do we guarantee compability between all the different code and data packages? And building/publishing releases could become far more of a burden on maintainers.

My gut feel is that doing this might cause more problems than it solves, but I'm happy to be proven wrong. The changes will need to work equally well for people using Moment Timezone in the browser via CDNs, on the server via npm, or in the browser via npm and a bundler such as webpack. An older discussion at #369 (comment) outlines some ideas.

We could also maintain the 0.x release line with the old data structure for a little while, to ease the transition to the new setup.

@mattjohnsonpint
Copy link
Contributor

First of all - THANK YOU so much for stepping up to help maintain Moment and Moment-Timezone. I've mostly stepped away from this library, but I've been noticing the work you're doing and appreciate it very much!

As for your proposed changes, I feel like you're the best one to make the call about what to do from here forward. I say, if you're willing to do the work and support it - then go for it. 👍

On the specific points:

  • Feel free to make it a 1.0.0 after your changes. The only reason I never did previously, was that the project was seen as experimental (as compared to Moment).
  • Peer dependencies - absolutely. Do it.
  • Removing the fixed-year files. Yes - agreed.
  • Changing the packed data format. I care less about this, but I'm not opposed.
  • Restricting the maximum future dates. Yes - do it. Nobody need time that far in the future, and as you said - it's unpredictable anyway.
  • Extract data into separate packages. Personally, I would choose not to. I think it may confuse people about what they have to install. But I don't feel strongly either way.

Again - thanks!

@mgol
Copy link

mgol commented Mar 29, 2023

Ad. 3c (extracting data to a separate package) - I don't know moment-timezone internals but would it require a lot of work to provide an optional new adapter that would only rely on data that browsers already embed instead of using a direct data file? Projects only supporting modern browsers but still stuck on Moment & moment-timezone would benefit from it, both by getting the size down and by not having to remember about updating the data file from time to time.

@mattjohnsonpint
Copy link
Contributor

mattjohnsonpint commented Mar 29, 2023

@mgol - It's a good idea conceptually. However in practice, it requires a lot of tricks to actually implement over the current Date and Intl APIs. By the time we implemented them all, the end-result would look a lot like Luxon.

A better plan might be to build Moment and Moment-TimeZone replacements on top of TC39's Temporal proposal. One could start developing now, using their published polyfill. Eventually the proposal will reach stage 4, and start being implemented in all major JS runtimes. Then you drop the polyfill, and release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants