perf: code-split codemirror & lottie + optimize the build for modern apps #21602

iamakulov · 2023-03-20T13:43:01Z

This PR might be easier to review commit-by-commit.

Description

This PR

switches the build target to modern browsers; this means stuff like class, async/await, etc is not transpiled anymore
code-splits away the code editor + sets up ESLint rules to enforce that
code-splits away Lottie + sets up ESLint rules to enforce that

In total, this saves 1.34 minified MBs (8.34 → 7 MB), getting rid of these modules (+ making others smaller, thanks to the modern build target):

Type of change

Chore (housekeeping or task changes that don't impact user perception)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions, so we can reproduce.
Please also list any relevant details for your test configuration.
Delete anything that is not important

Manual:
- testing the code editor (including by emulating slow connections);
- testing the lottie player (confetti animation + welcome tour)

Test Plan

Add Testsmith test cases links that relate to this PR

Issues raised during DP testing

Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR)

Checklist:

Dev activity

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
PR is being merged under a feature flag

QA activity:

Test plan has been approved by relevant developers
Test plan has been peer reviewed by QA
Cypress test cases have been added and approved by either SDET or manual QA
Organized project review call with relevant stakeholders after Round 1/2 of QA
Added Test Plan Approved label after reveiwing all Cypress test

(This retains stuff like classes, async functions, etc as-is)

This commit - replaces all usages of CodeEditor with LazyCodeEditor - makes LazyCodeEditor lazy-load the CodeEditor chunk on demand

…itor This removes the need for DynamicTextFieldControl.tsx to pull in code (`getLineCommentString()`) from the CodeEditor directory

AutocompleteDataType is imported by ~40 files around the codebase. Previously, it was defined in `utils/autocomplete/CodemirrorTernService`, which relies on the CodeMirror import. This meant that any file importing AutocompleteDataType also imported CodeMirror. This commit fixes that by moving AutocompleteDataType into a separate file.

…ports

iamakulov · 2023-03-20T13:46:26Z

app/client/.eslintrc.js

+    // `no-restricted-imports` is disabled, as recommended in https://typescript-eslint.io/rules/no-restricted-imports/.
+    // Please use @typescript-eslint/no-restricted-imports below instead.
+    "no-restricted-imports": "off",
+    "@typescript-eslint/no-restricted-imports": [
+      "error",
+      {
+        paths: [
+          {
+            name: "codemirror",
+            message:
+              "Reason: If you want to call CodeMirror.on(), CodeMirror.Pos(), or similar functions, please don’t import CodeMirror directly. (This will cause it to be bundled in the main chunk.) Instead, assuming your function has access to CodeMirror’s editor or doc, use getCodeMirrorNamespaceFromEditor() or getCodeMirrorNamespaceFromDoc() functions to get the CodeMirror namespace from the editor or the doc.",
+            // Allow type imports as they don’t lead to bundling the dependency
+            allowTypeImports: true,
+          },
+        ],
+        paths: [
+          {
+            name: "lottie-web",
+            message:
+              "Reason: Please don’t import lottie directly as it’s very large. Instead, use the utils/lazyLottie wrapper.",
+            // Allow type imports as they don’t lead to bundling the dependency
+            allowTypeImports: true,
+          },
+        ],
+      },
+    ],
+    // Annoyingly, the `no-restricted-imports` rule doesn’t allow to restrict imports of
+    // `editorComponents/CodeEditor` but not `editorComponents/CodeEditor/*`: https://stackoverflow.com/q/64995811/1192426
+    // So we’re using `no-restricted-syntax` instead.
+    "no-restricted-syntax": [
+      "error",
+      {
+        // Match all
+        //   - `import` statements
+        //   - that are not `import type` statements – we allow type imports as they don’t lead to bundling the dependency
+        //   - that import `editorComponents/CodeEditor` or `editorComponents/CodeEditor/index` but not `editorComponents/CodeEditor/<anything else>`
+        // Note: using `\\u002F` instead of `/` due to https://eslint.org/docs/latest/extend/selectors#known-issues
+        selector:
+          "ImportDeclaration[importKind!='type'][source.value=/editorComponents\\u002FCodeEditor(\\u002Findex)?$/]",
+        message:
+          "Please don’t import CodeEditor directly – this will cause it to be bundled in the main chunk. Instead, use the LazyCodeEditor component.",
+      },
+    ],


The diff for this file is a bit annoying because .json got renamed into .js, and git didn’t match them. But the only new lines are these...

iamakulov · 2023-03-20T13:46:36Z

app/client/.eslintrc.js

+eslintConfig.overrides = [
+  // For CodeEditor, disable CodeEditor- and CodeMirror-specific import rules
+  {
+    files: ["**/components/editorComponents/CodeEditor/**/*"],
+    rules: {
+      "@typescript-eslint/no-restricted-imports":
+        getRestrictedImportsOverrideForCodeEditor(),
+      "no-restricted-syntax": getRestrictedSyntaxOverrideForCodeEditor(),
+    },
+  },
+];
+
+function getRestrictedImportsOverrideForCodeEditor() {
+  const [errorLevel, existingRules] =
+    eslintConfig.rules["@typescript-eslint/no-restricted-imports"];
+
+  const newPatterns = (existingRules.patterns ?? []).filter(
+    (i) => i.group[0] !== "**/components/editorComponents/CodeEditor",
+  );
+  const newPaths = (existingRules.paths ?? []).filter(
+    (i) => i.name !== "codemirror",
+  );
+
+  if (newPatterns.length === 0 && newPaths.length === 0) {
+    return ["off"];
+  }
+
+  return [errorLevel, { patterns: newPatterns, paths: newPaths }];
+}
+
+function getRestrictedSyntaxOverrideForCodeEditor() {
+  const [errorLevel, ...existingRules] =
+    eslintConfig.rules["no-restricted-syntax"];
+
+  const newRules = existingRules.filter(
+    (i) =>
+      i.selector !==
+      "ImportDeclaration[source.value=/editorComponents\\u002FCodeEditor(\\u002Findex)?$/]",
+  );
+
+  if (newRules.length === 0) {
+    return ["off"];
+  }
+
+  return [errorLevel, ...newRules];
+}


...and these

iamakulov · 2023-03-20T14:05:06Z

app/client/package.json

-    ">0.2%",
-    "not dead",
-    "not ie <= 11",
-    "not op_mini all"
+    "defaults and supports es6-module"


Why the switch?

If you take the old Browserslist query (>0.2%, not dead, not ie <= 11, not op_mini all) and paste it into the Babel REPL, you’ll see that this query makes classes and async functions transpile down to ES5:

The reason for this can be seen at browsersl.ist. This query matches Android 4, which has a 0.3% browser share and does not support any of the modern JS features:

To solve this, instead of adding yet another exception, I decided to switch to defaults and supports es6-module. defaults means > 0.5%, last 2 versions, Firefox ESR, not dead, whereas supports es6-module excludes weird ancient browsers like Opera Mini or KaiOS 2.5.

This switch keeps all the stuff like async/await as-is and saves 0.67 minified MBs in the main bundle alone.

iamakulov · 2023-03-20T14:06:36Z

app/client/src/assets/lottie/binding.json

(I removed unused Lottie animations...)

iamakulov · 2023-03-20T14:07:45Z

app/client/src/assets/lottie/guided-tour-indicator.json.txt

(...and renamed existing ones to .txt so webpack can process them as an asset/resource module instead of bundling the JSON right into the app.)

iamakulov · 2023-03-20T14:27:32Z

app/client/src/components/editorComponents/LazyCodeEditor/index.tsx

+type LazyCodeEditorState =
+  // The initial state when the state machine is initialized
+  | "idle"
+  // The state when the CodeEditor chunk is loading
+  | "loading"
+  // The state when the CodeEditor chunk is loading, but the user has interacted with the placeholder
+  | "loading-interacted"
+  // The state when we’re waiting for the idle callback to fire to render CodeEditor
+  | "waiting-idle-callback"
+  // The state when the CodeEditor must be rendered
+  | "active"
+  // The state when the CodeEditor must be rendered and made focused
+  | "active-focused"
+  // The state when the CodeEditor chunk failed to load
+  | "error";


To code-split the code editor, I re-used the existing LazyCodeEditorWrapper component.

The component already had logic to

rendering the code editor with requestIdleCallback()

and to speed up that render when the user interacts with the placeholder

With the addition of lazy-loading, the logic got really complex, so I decided to implement it as a barebones state machine. The comments should (hopefully) make it clear what’s going on, but just for the reference, here’s a full diagram of all states: https://stately.ai/registry/editor/b6ae19f0-6adb-47f7-a00b-dac5cf9a61bd?machineId=8e0d268e-57b2-4fe5-8f65-cb363a57f141

KelvinOm · 2023-03-22T08:59:41Z

app/client/src/Globals.d.ts

@@ -1 +1,6 @@
 declare module "*.module.css";
+
+declare module "*.txt" {


Why is this necessary?

For this: https://github.com/appsmithorg/appsmith/pull/21602/files/0b750838576786578dd2b095acab183cb3f60e65#diff-4463e6b504253024bc05a7214c25934113b5c13ef05e2e6020cdb463c9574d76

This is needed to give import like

appsmith/app/client/src/utils/helpers.tsx

Line 7 in 2cf0a35

import welcomeConfettiAnimationURL from "assets/lottie/welcome-confetti.json.txt";

correct types.

KelvinOm · 2023-03-22T09:10:40Z

app/client/src/components/editorComponents/LazyCodeEditor/index.tsx

@@ -0,0 +1,236 @@
+import React, { useState, useEffect, useRef, useCallback } from "react";


@hetunandu Could you review this a code?

Looks good to me

KelvinOm · 2023-03-22T09:12:56Z

app/client/.eslintrc.js

@@ -0,0 +1,179 @@
+/** @type {import('eslint').Linter.Config} */


Why is this necessary?

In VSCode (and maybe other IDEs?), this makes the IDE provide type-based autocomplete for the ESLint config, which is much better.

With /** @type **/:

Without /** @type **/:

In WebStorm, it is the same with and without type

It also seems to be necessary only for eslintrc.js, in .eslintrc.json intellisense is working fine.

In any case, please leave a comment on what it is.

KelvinOm · 2023-03-22T09:22:28Z

app/client/.eslintrc.js

+    // Annoyingly, the `no-restricted-imports` rule doesn’t allow to restrict imports of
+    // `editorComponents/CodeEditor` but not `editorComponents/CodeEditor/*`: https://stackoverflow.com/q/64995811/1192426
+    // So we’re using `no-restricted-syntax` instead.
+    "no-restricted-syntax": [


We inherit this config here and here, and in the future we will also use the general rules for the rest of the packages. So I have to ask do we really need these rules for other dependencies?

I would suggest separating the general rules and inheriting them here and in packages.

How about this? 0f463db

This splits out the base ESLint config (src/.eslintrc.base.json) and makes the following packages use it:

client

client/cypress

client/packages/storybook

shared/ast (🆕)

This definitely creates a bunch of new ESLint errors (all or almost all of them formatting-based), but I’d rather resolve them when we’re merging this upstream, since #21442 also tuned the ESLint configs somewhat.

Yes, it's better that way. I think shared/ast can be skipped for now. I would like to move it to packages.

As for formatting, please pay attention to #21679.

(I’ll keep the ESLint changes for shared/ast for now. I added them for consistency [to make every project with .eslintrc.json inherit a shared config], and, as I understand, they won’t complicate moving shared/ast to packages.

But let me know if I misunderstood, and the inheritance there is actively harmful.)

Thanks! The config will not complicate the movement. )

SatishGandham · 2023-03-23T10:21:57Z

app/client/src/components/editorComponents/LazyCodeEditor/index.tsx

+          if (CachedCodeEditor) {
+            this.state = "waiting-idle-callback";
+          } else {
+            this.state = "loading";


@iamakulov ,
This is the only place where we change the state to loading, and the state has to be loading to trigger the import. If the state is idle handleStateChange is exiting, where exactly does the state change to loading?

Here!

appsmith/app/client/src/components/editorComponents/LazyCodeEditor/index.tsx

Lines 186 to 188 in 4504a62

useEffect(() => {

stateMachine.current.transition("RENDERED");

}, []);

Note that I intentionally trigger the event from useEffect. It’s not the fastest approach (we’re delaying loading until useEffect() fires), but running side effects from render() is not 100% safe, especially with concurrent React 18 features.

(Although I just tried, and I can’t come up with a case where that will actually lead to bugs in practice. But better safe than sorry I guess.)

iamakulov added 7 commits March 20, 2023 13:28

Build Appsmith for modern browsers only

b46426c

(This retains stuff like classes, async functions, etc as-is)

CodeEditor, step 1: code-split CodeEditor away

4504a62

This commit - replaces all usages of CodeEditor with LazyCodeEditor - makes LazyCodeEditor lazy-load the CodeEditor chunk on demand

CodeEditor, step 2: move the getLineCommentString logic into CodeEd…

f420a05

…itor This removes the need for DynamicTextFieldControl.tsx to pull in code (`getLineCommentString()`) from the CodeEditor directory

CodeEditor, step 4: replace CodeMirror imports with dynamic retrieval

5276005

CodeEditor, step 5: prohibit problematic CodeMirror and CodeEditor im…

881832e

…ports

Code-split Lottie and Lottie animations away

0b75083

iamakulov force-pushed the perf/codemirror-lottie-etc branch from aa26305 to 0b75083 Compare March 20, 2023 13:44

iamakulov commented Mar 20, 2023

View reviewed changes

iamakulov added 2 commits March 20, 2023 19:46

Fix an incorrect ESLint setup

5bec936

Use the esnext field to resolve package entrypoints

19c0c8d

SatishGandham requested a review from KelvinOm March 21, 2023 08:55

Merge branch 'bundle-optimization' into perf/codemirror-lottie-etc

2cf0a35

KelvinOm reviewed Mar 22, 2023

View reviewed changes

Split out the base ESLint config + use it in shared/ast

0f463db

KelvinOm approved these changes Mar 22, 2023

View reviewed changes

Clarify why the @type comment exists

4032cd0

SatishGandham merged commit f8a2c76 into bundle-optimization Mar 23, 2023

SatishGandham deleted the perf/codemirror-lottie-etc branch March 23, 2023 07:02

SatishGandham reviewed Mar 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: code-split codemirror & lottie + optimize the build for modern apps #21602

perf: code-split codemirror & lottie + optimize the build for modern apps #21602

iamakulov commented Mar 20, 2023 •

edited

Loading

iamakulov Mar 20, 2023 •

edited

Loading

iamakulov Mar 20, 2023

iamakulov Mar 20, 2023

iamakulov Mar 20, 2023

iamakulov Mar 20, 2023

iamakulov Mar 20, 2023

KelvinOm Mar 22, 2023

iamakulov Mar 22, 2023

KelvinOm Mar 22, 2023

hetunandu Mar 22, 2023

KelvinOm Mar 22, 2023

iamakulov Mar 22, 2023

KelvinOm Mar 22, 2023

KelvinOm Mar 22, 2023

iamakulov Mar 22, 2023

KelvinOm Mar 22, 2023

iamakulov Mar 22, 2023

KelvinOm Mar 22, 2023

SatishGandham Mar 23, 2023

iamakulov Mar 23, 2023

		@@ -1 +1,6 @@
		declare module "*.module.css";

		declare module "*.txt" {

		@@ -0,0 +1,236 @@
		import React, { useState, useEffect, useRef, useCallback } from "react";

		@@ -0,0 +1,179 @@
		/** @type {import('eslint').Linter.Config} */

	useEffect(() => {
	stateMachine.current.transition("RENDERED");
	}, []);

perf: code-split codemirror & lottie + optimize the build for modern apps #21602

perf: code-split codemirror & lottie + optimize the build for modern apps #21602

Conversation

iamakulov commented Mar 20, 2023 • edited Loading

Description

Type of change

How Has This Been Tested?

Test Plan

Issues raised during DP testing

Checklist:

Dev activity

QA activity:

iamakulov Mar 20, 2023 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamakulov commented Mar 20, 2023 •

edited

Loading

iamakulov Mar 20, 2023 •

edited

Loading