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

perf: Lazy load moment-timezone #29791

Merged
merged 5 commits into from
Aug 5, 2024
Merged

Conversation

kgabryje
Copy link
Member

SUMMARY

moment-timezone library is over 700kb and it is only used in alerts/reports modal. Loading it by default slows down dashboard's loading time.
This PR implements lazy loading this library.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Notice the lack of ~800kb network activity in the "after" screenshot

Before:

Screenshot 2024-07-31 at 15 51 32

After:

Screenshot 2024-07-31 at 16 36 23

TESTING INSTRUCTIONS

  1. Open a dashboard
  2. Verify that the moment-timezone chunk was not loaded
  3. 3 dots menu -> Manage email report -> Set up an email report
  4. Verify that the timezone picker is working correctly

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@dosubot dosubot bot added alert-reports Namespace | Anything related to the Alert & Reports feature dashboard:performance Related to Dashboard performance labels Jul 31, 2024
@kgabryje kgabryje force-pushed the perf/moment-timezone branch from 51aeb4e to 98d9c35 Compare July 31, 2024 16:25
@michael-s-molina
Copy link
Member

Thanks for working on this type of task! That will bring huge benefit to the community.

@mistercrunch
Copy link
Member

Glad to see "frontend-infra" getting some ❤️ !

@mistercrunch
Copy link
Member

mistercrunch commented Jul 31, 2024

Hey @dosu, I'm curious if you can point to libraries that are popular drop-in/easy replacement for moment-timezone and/or moment.js

@rusackas
Copy link
Member

rusackas commented Jul 31, 2024

Code LGTM, but have you looked at what's covered by tests (or not)? Some suggestions that were TOTALLY NOT generated by AI ;)

  • Dynamic import of moment-timezone: Test that the momentLib state is initially null and then populated after the component mounts.
  • Loading state: Verify that the Loading component is rendered while momentLib is still null.
  • Memoized calculations: Test that TIMEZONE_OPTIONS, TIMEZONE_OPTIONS_SORT_COMPARATOR, and validTimezone are correctly calculated once momentLib is loaded.
  • Timezone selection: Ensure that timezone selection still works as expected after the refactoring.
  • Effect for updating timezone: Test that onTimezoneChange is called with the correct timezone when the component mounts or when the timezone prop changes.

@rusackas
Copy link
Member

Hey @dosu, I'm curious if you can point to libraries that are popular drop-in/easy replacement for moment-timezone and/or moment.js

Intl.DateTimeFormat() might be a replacement. Vanilla JS FTW!

@kgabryje
Copy link
Member Author

@mistercrunch @rusackas replacing moment is going to be a big effort as it’s used everywhere, but it’s definitely going to be worthwhile for performance - moment is big

@mistercrunch
Copy link
Member

Yeah it's used in 30+ modules ->

$ git grep "import.*moment" superset-frontend/src/
superset-frontend/src/SqlLab/components/QueryTable/index.tsx:import moment from 'moment';
superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx:import moment from 'moment';
superset-frontend/src/components/CachedLabel/TooltipContent.test.tsx:import moment from 'moment';
superset-frontend/src/components/CachedLabel/TooltipContent.tsx:import moment from 'moment';
superset-frontend/src/components/Chart/chartAction.js:import moment from 'moment';
superset-frontend/src/components/LastUpdated/index.tsx:import moment, { Moment, MomentInput } from 'moment';
superset-frontend/src/components/ListView/Filters/DateRange.tsx:import moment, { Moment } from 'moment';
superset-frontend/src/components/Timer/Timer.stories.tsx:import moment from 'moment';
superset-frontend/src/components/TimezoneSelector/TimezoneSelector.test.tsx:import moment from 'moment-timezone';
superset-frontend/src/components/TimezoneSelector/index.tsx:import moment from 'moment-timezone';
superset-frontend/src/dashboard/components/Header/index.jsx:import moment from 'moment';
superset-frontend/src/dashboard/components/OverwriteConfirm/OverwriteConfirmModal.tsx:import moment from 'moment';
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:import moment from 'moment';
superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx:import moment from 'moment';
superset-frontend/src/explore/components/controls/DateFilterControl/components/CustomFrame.tsx:import { Moment } from 'moment';
superset-frontend/src/explore/components/controls/DateFilterControl/utils/constants.ts:import moment from 'moment';
superset-frontend/src/explore/components/controls/DateFilterControl/utils/dateParser.ts:import moment, { Moment } from 'moment';
superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx:import moment, { Moment } from 'moment';
superset-frontend/src/features/allEntities/AllEntitiesTable.tsx:import moment from 'moment';
superset-frontend/src/features/annotations/AnnotationModal.tsx:import moment from 'moment';
superset-frontend/src/features/datasets/AddDataset/EditDataset/UsageTab/index.tsx:import moment from 'moment';
superset-frontend/src/features/home/ActivityTable.tsx:import moment from 'moment';
superset-frontend/src/pages/AlertReportList/index.tsx:import moment from 'moment';
superset-frontend/src/pages/AnnotationList/index.tsx:import moment from 'moment';
superset-frontend/src/pages/ExecutionLogList/index.tsx:import moment from 'moment';
superset-frontend/src/pages/QueryHistoryList/index.tsx:import moment from 'moment';
superset-frontend/src/preamble.ts:import moment from 'moment';
superset-frontend/src/utils/dates.js:import moment from 'moment';
superset-frontend/src/visualizations/TimeTable/SparklineCell.tsx:import moment from 'moment';

$ git grep "import.*moment" superset-frontend/plugins/
superset-frontend/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js:import moment from 'moment';
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts:import moment from 'moment';
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/utils.ts:import moment from 'moment';
superset-frontend/plugins/plugin-chart-handlebars/src/components/Handlebars/HandlebarsViewer.tsx:import moment from 'moment';

Curious how many different functions we use.

From memory working on bundle sizes in the past, I think they have a large i18n module that we don't really use (I think). I'm not sure if we're shaking that off properly or not, but could be something to look at and make sure we're not importing things we don't need.

@kgabryje
Copy link
Member Author

Yeah most of it is a locale submodule. I’ll see if we can shake it off

@rusackas
Copy link
Member

rusackas commented Aug 1, 2024

I’ll see if we can shake it off

@kgabryje kgabryje force-pushed the perf/moment-timezone branch from 1357a3e to de2f974 Compare August 1, 2024 16:20
@kgabryje
Copy link
Member Author

kgabryje commented Aug 1, 2024

@mistercrunch @rusackas Do we have a list of locales supported in Superset? We can remove the unsupported ones with a webpack plugin (source)

Those are the locales supported in a date picker component - should we stick to those and set en-US as a fallback?

image

@rusackas
Copy link
Member

rusackas commented Aug 1, 2024

I wonder if they should be 1:1 with the languages that are enabled in the config file. That'd make it dynamic, support expansion when people add languages (which does happen), and keep things as minimal as possible. Or we could even move this to a similar config.

@mistercrunch
Copy link
Member

output is a bit messy, but here are our language packs ->

$ ls -d superset/translations/*
superset/translations/__init__.py               superset/translations/it                        superset/translations/sk
superset/translations/__pycache__               superset/translations/ja                        superset/translations/sl
superset/translations/ar                        superset/translations/ko                        superset/translations/tr
superset/translations/babel.cfg                 superset/translations/messages.pot              superset/translations/uk
superset/translations/de                        superset/translations/nl                        superset/translations/utils.py
superset/translations/empty_language_pack.json  superset/translations/pt                        superset/translations/zh
superset/translations/en                        superset/translations/pt_BR                     superset/translations/zh_TW
superset/translations/es                        superset/translations/requirements.txt
superset/translations/fr                        superset/translations/ru

@kgabryje
Copy link
Member Author

kgabryje commented Aug 4, 2024

@mistercrunch @rusackas Managed to tree shake the locales 🙂
Before:

Screenshot 2024-08-04 at 14 57 23

After:

Screenshot 2024-08-04 at 17 23 14

@kgabryje kgabryje merged commit 9c058fe into apache:master Aug 5, 2024
34 checks passed
WanjohiWanjohi pushed a commit to IDinsight/surveystream_superset_source that referenced this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alert-reports Namespace | Anything related to the Alert & Reports feature dashboard:performance Related to Dashboard performance dependencies:npm size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants