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

Generate smaller bundle options with limited timezone data #73

Merged
merged 9 commits into from Jun 16, 2019

Conversation

jkillian
Copy link
Contributor

@jkillian jkillian commented Jun 13, 2019

Fixes #67.

Changes

This PR changes the build process such that three new dev/prod bundles are produced:

  • js-joda-timezone-10-year-range.js which includes timezone data for +- five years from the current version's release
  • js-joda-timezone-1970-2030.js which includes timezone data for covers from 1970 to 2030
  • js-joda-timezone-2012-2022.js which includes timezone data for covers from 2012 to 2022

These bundles intentionally echo what moment-timezone provides.

Users can choose to use these bundles in their code by simply importing them:

import 'js-joda-timezone/dist/js-joda-timezone-1970-2030'

Here are the file sizes of the new bundles compared to the current bundle:

Minified bundle sizes:

bundle size
all 911 KB
10-year-range 34 KB
1970 - 2030 150 KB
2012 - 2022 34 KB

Build Steps

  • Copy over latest data from moment-timezone:
    • moment-timezone's unpacked latest.json should be copied over to data/unpacked/latest.json.
    • moment-timezone's dated packed data, such as 2018h.json, can be copied over to data/packed/<name> if it isn't yet.
  • Run npm run transform-data to generate all the data/packed/latest*.json files. (Note that CI doesn't have to run this step since I checked the generated files in.)
  • Run npm run build-dist to build all the .js bundles based on the latest*.json files available.

A few caveats / questions for maintainers:

  • npm was acting strangely for me and modifying much too mock of the package-lock.json file so I had to manually add -p it and it's possible things went wrong. Feel free to change it or make any other changes to this PR also, or comment and I'm happy to make changes.
  • Do we need to change tests at all to account for these new builds?
  • How does js-joda react if it doesn't have timezone data available? For example, if you're using js-joda-timezone-10-year-range.js and you have a ZonedDateTime outside of that range, is the behavior graceful?
  • I added data/packed/2018h.json through data/packed/2019a.json while I was modifying things, but happy to remove them if that's too orthogonal to this PR.

};

const writeData = (suffix, startYear = 0, endYear = 9999) => {
const packedData = moment.tz.filterLinkPack(tzdbLatest, startYear, endYear);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: this is the same way that moment-timezone produces its smaller bundles.

@coveralls
Copy link

coveralls commented Jun 13, 2019

Coverage Status

Coverage remained the same at 98.16% when pulling 46c66c5 on JKillian:smaller-builds into 72fe18c on js-joda:master.

@mlc
Copy link

mlc commented Jun 14, 2019

i'm no expert in the relevant details, but this looks exactly like what I was wanting 😸

@pithu pithu self-requested a review June 14, 2019 15:33
@pithu
Copy link
Member

pithu commented Jun 16, 2019

Hello @jkillian,

thanks a lot for using js-joda and contributing. Your PR is looking great, i probably would have done it very similar. What would be nice to have, is a Howto build timezone data, as describe by you in the Build Steps.

To your questions:

npm was acting strangely for me and modifying much too mock of the package-lock.json ...
The differences in the package-lock.json are probably because we are using different node nvm version. It should be ok to have some small difference.

Do we need to change tests at all to account for these new builds?
Having karma tests for all build version in dist would be also nice to have, but the test need to be adapted for that, because some contain tests for older tzdb entries.

How does js-joda react if it doesn't have timezone data available?
I did not test it yet, but it should be no problem, i expect that js-joda will use the earliest available tzdb data entry. So the result might be incorrect, but calculation should not fail.

@pithu pithu merged commit 2b1df89 into js-joda:master Jun 16, 2019
@jkillian jkillian deleted the smaller-builds branch June 17, 2019 03:09
@jkillian
Copy link
Contributor Author

Amazing! Thanks for the merge @pithu. Let me know when you release and we'll pick up the new version in our codebase.

Having karma tests for all build version in dist would be also nice to have, but the test need to be adapted for that, because some contain tests for older tzdb entries.

Agreed! For a later PR perhaps.

I did not test it yet, but it should be no problem, i expect that js-joda will use the earliest available tzdb data entry. So the result might be incorrect, but calculation should not fail.

I think this is the correct behavior, sounds good!


Also, I'd love to be able to make some space-savings over in js-joda-locale also! Let me know over in that thread if you have any ideas for improvements.

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

Successfully merging this pull request may close these issues.

wishlist: offer options for smaller data bundles
4 participants