feat: move ace-editor and mathjs to async modules #10837

ktmud · 2020-09-11T05:27:20Z

SUMMARY

Follow up on #10831, move brace and mathjs to async modules so that the initial page load for ~~dashboards~~ most pages can be faster.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

On a dashboard page, brace loads only after users add a markdown widget to dashboard:

Optimized bundle config further helps reduce the overall bundle size:

Initial page load time with production assets and without cache was reduced by 30% (9.8s -> 6.9s in Fast 3G network).

On Explore and CRUD page, brace and mathjs are also loaded on demand.

TEST PLAN

Manual testing + a couple of cypress tests

Expected behaviors:

Dashboards should not load brace.js and mathjs by default.
Entering edit mode in dashboards with Markdown widgets should preload brace.js, even when users haven't started editing the markdown.
Dashboards with Line charts should load mathjs automatically.
In explore view, mathjs will load only for charts that support annotation layers.

ADDITIONAL INFORMATION

codecov-commenter · 2020-09-11T08:58:35Z

Codecov Report

Merging #10837 into master will increase coverage by 2.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #10837      +/-   ##
==========================================
+ Coverage   59.32%   61.34%   +2.02%     
==========================================
  Files         776      380     -396     
  Lines       37060    24086   -12974     
  Branches     3310        0    -3310     
==========================================
- Hits        21986    14776    -7210     
+ Misses      14891     9310    -5581     
+ Partials      183        0     -183

Flag	Coverage Δ
#cypress	`?`
#python	`61.34% <ø> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/db_engine_specs/presto.py	`81.61% <0.00%> (-0.68%)`	⬇️
...set-frontend/src/dashboard/util/injectCustomCss.js
...board/util/getSelectedChartIdForFilterScopeTree.js
.../src/SqlLab/components/EstimateQueryCostButton.jsx
...uperset-frontend/src/components/TooltipWrapper.jsx
...frontend/src/dashboard/actions/dashboardFilters.js
...src/dashboard/components/DeleteComponentButton.jsx
...end/src/dashboard/util/getKeyForFilterScopeTree.js
.../src/explore/components/FilterDefinitionOption.jsx
...nd/src/dashboard/util/getComponentWidthFromDrop.js
... and 371 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 045335a...21616b3. Read the comment docs.

ktmud

@kristw @etr2460 @graceguo-supercat This is ready for review. Would you mind taking a look?

ktmud · 2020-09-11T16:57:17Z

superset-frontend/src/components/AsyncEsmComponent.tsx

+    // height is specified.
+    (height && (
+      <div style={{ width, height }}>
+        {showLoadingForImport && <Loading position="floating" />}


This is for displaying a loading spinner while the async module is still loading:

E.g.:

Normally users wouldn't see this because most places we used the AsyncEsmComponent have on-demand preloading.

ktmud · 2020-09-11T21:30:33Z

superset-frontend/webpack.config.js

  output.filename = '[name].[chunkhash].entry.js';
  output.chunkFilename = '[name].[chunkhash].chunk.js';
+} else {
+  output.filename = '[name].[chunkhash].entry.js';
+  output.chunkFilename = '[chunkhash].chunk.js';


Don't add names to chunkFiles (they are just single digit numbers anyway).

ktmud · 2020-09-11T21:31:00Z

superset-frontend/webpack.config.js

              'redux',
              'react-redux',
              'react-hot-loader',
              'react-select',
              'react-sortable-hoc',
              'react-virtualized',
              'react-table',
+              'react-ace',


react-ace itself is quite small.

ktmud · 2020-09-11T21:31:39Z

superset-frontend/webpack.config.js

-          name: 'brace',
-          test: /\/node_modules\/(brace|react-ace)\//,
-          priority: 40,
-        },


Different brace modes/themes now load separately and asynchronously.

ktmud · 2020-09-11T21:33:08Z

superset-frontend/src/explore/components/controls/TextAreaControl.jsx

          maxLines={inModal ? 1000 : this.props.maxLines}
-          onChange={this.onAceChange.bind(this)}
+          onChange={this.onAceChangeDebounce}


Added debounce to text inputs in Explore view's Datasource editor.

ktmud · 2020-09-11T21:33:33Z

superset-frontend/src/explore/components/controls/TextAreaControl.jsx

          editorProps={{ $blockScrolling: true }}
-          enableLiveAutocompletion


Removed this prop according to a console warning from Ace.

ktmud · 2020-09-11T21:37:14Z

superset-frontend/src/components/AsyncAceEditor.tsx

+              prefix: string,
+              callback: (error: null, wordList: object[]) => void,
+            ) => {
+              if ((session.getMode() as TextMode).$id === `ace/mode/${mode}`) {


Enable completion for only the related language.

ktmud · 2020-09-11T21:38:18Z

superset-frontend/src/components/AsyncAceEditor.tsx

+ */
+export default function AsyncAceEditor(
+  aceModules: AceModule[],
+  { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},


Set default tabSize for all AceEditor. Previously there were some places where tabSize was not set and would default to 4.

mistercrunch · 2020-09-12T06:30:32Z

superset-frontend/cypress-base/cypress/integration/explore/control.test.ts

superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts

superset-frontend/src/dashboard/components/CssEditor.jsx

rusackas

LGTM! The cleanup this brings, and the performance improvement, are both appreciated.

Left some general questions & comments you're already addressing, so thanks for that.

rusackas · 2020-09-15T00:06:41Z

superset-frontend/src/explore/components/controls/AnnotationLayerControl.jsx

+const AnnotationLayer = AsyncEsmComponent(
+  () => import('./AnnotationLayer'),
+  // size of overlay inner content
+  () => <div style={{ width: 450, height: 368 }} />,


Could/should these be based on % values or use a flexbox layout (using the built-in "stretch" features) to avoid hard-coding these dimensions?

The goal here is to make sure the popover renders at the right position so that the left caret always points to the popover trigger when the content of the popover is fully loaded. We wouldn't want the popover to "stretch" since there is no way to update its position after it was re-rendered.

kristw · 2020-09-15T19:08:35Z

superset-frontend/webpack.config.js

              'moment',
              'jquery',
              'core-js.*',
              '@emotion.*',
-              'd3.*',
+              'd3',
+              'd3-(array|color|scale||interpolateformat|selection|collection|time|time-format)',


typo

||interpolateformat => |interpolate|format

Wow, just wow... Thanks for spotting this!

Load markdown async Add a test case for markdown edit

Follow up on apache#10831, move brace and mathjs to async modules so that the initial page load for dashboards most pages can be faster.

ktmud requested review from mistercrunch, rusackas, kristw, etr2460 and graceguo-supercat September 11, 2020 05:27

pull-request-size bot added the size/L label Sep 11, 2020

ktmud force-pushed the async-mathjs branch from 6c76db5 to 36716b5 Compare September 11, 2020 07:13

pull-request-size bot added size/XL and removed size/L labels Sep 11, 2020

ktmud force-pushed the async-mathjs branch 2 times, most recently from a9f4ae0 to ee48ef6 Compare September 11, 2020 17:00

ktmud marked this pull request as draft September 11, 2020 19:03

ktmud force-pushed the async-mathjs branch 2 times, most recently from cce2701 to c6aa954 Compare September 11, 2020 20:43

ktmud marked this pull request as ready for review September 11, 2020 21:18

ktmud force-pushed the async-mathjs branch 2 times, most recently from 2582cb9 to bfc0805 Compare September 11, 2020 22:07

ktmud commented Sep 11, 2020

View reviewed changes

ktmud force-pushed the async-mathjs branch from e088d50 to 5e0a128 Compare September 12, 2020 00:54

ktmud force-pushed the async-mathjs branch from 5e0a128 to 25b085b Compare September 14, 2020 20:53

rusackas reviewed Sep 14, 2020

View reviewed changes

superset-frontend/cypress-base/cypress/integration/explore/control.test.ts Show resolved Hide resolved

rusackas reviewed Sep 14, 2020

View reviewed changes

superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts Show resolved Hide resolved

rusackas reviewed Sep 14, 2020

View reviewed changes

superset-frontend/src/dashboard/components/CssEditor.jsx Show resolved Hide resolved

ktmud force-pushed the async-mathjs branch from 25b085b to dff4409 Compare September 15, 2020 00:03

rusackas approved these changes Sep 15, 2020

View reviewed changes

ktmud force-pushed the async-mathjs branch from dff4409 to 21616b3 Compare September 15, 2020 17:18

pull-request-size bot added size/XXL and removed size/XL labels Sep 15, 2020

kristw approved these changes Sep 15, 2020

View reviewed changes

ktmud force-pushed the async-mathjs branch from cc8da66 to 152d41e Compare September 15, 2020 19:13

ktmud added 11 commits September 15, 2020 13:34

feat: load AnnotationLayer and mathjs on demand

4d42f89

Make brace async for dashboard page

0db8bcb

Load markdown async Add a test case for markdown edit

Make it work for datasource editor

c8b2f66

Better preload and placeholder

7b4b958

Introduce a consolidated AsyncAceEditor component

6ce9161

Try to fix Cypress tests

d20703a

Split bootstrap-slider to vendors, too

aed8b13

Create custom placeholder for AceEditor in SQL Lab

6decb33

Add test for vizType control

3c801d4

Fix a typo

0d1a9ae

Try to fix AdhocFilters test

4477fdd

ktmud force-pushed the async-mathjs branch from 152d41e to 4477fdd Compare September 15, 2020 20:41

ktmud merged commit 0129c42 into apache:master Sep 15, 2020

ktmud deleted the async-mathjs branch September 15, 2020 21:12

ktmud mentioned this pull request Nov 12, 2020

feat: add ECharts BoxPlot chart #11199

Merged

6 tasks

AAfghahi mentioned this pull request Oct 20, 2021

fix(AlertReportModal): Text Area Change #17176

Merged

9 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: move ace-editor and mathjs to async modules #10837

feat: move ace-editor and mathjs to async modules #10837

ktmud commented Sep 11, 2020 •

edited

Loading

codecov-commenter commented Sep 11, 2020 •

edited

Loading

ktmud left a comment •

edited

Loading

ktmud Sep 11, 2020

ktmud Sep 11, 2020

ktmud Sep 11, 2020

ktmud Sep 11, 2020

ktmud Sep 11, 2020

ktmud Sep 11, 2020

ktmud Sep 11, 2020

ktmud Sep 11, 2020

mistercrunch commented Sep 12, 2020

rusackas left a comment

rusackas Sep 15, 2020

ktmud Sep 15, 2020

kristw Sep 15, 2020

ktmud Sep 15, 2020

		editorProps={{ $blockScrolling: true }}
		enableLiveAutocompletion

feat: move ace-editor and mathjs to async modules #10837

feat: move ace-editor and mathjs to async modules #10837

Conversation

ktmud commented Sep 11, 2020 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov-commenter commented Sep 11, 2020 • edited Loading

Codecov Report

ktmud left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch commented Sep 12, 2020

rusackas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud commented Sep 11, 2020 •

edited

Loading

codecov-commenter commented Sep 11, 2020 •

edited

Loading

ktmud left a comment •

edited

Loading