From cf15fe0d03529432e1ef1741036ab5b60af197d2 Mon Sep 17 00:00:00 2001
From: David Aaron Suddjian <1858430+suddjian@users.noreply.github.com>
Date: Mon, 7 Jun 2021 14:04:56 -0700
Subject: [PATCH] fix(dashboard): custom css should be removed on unmount
 (#15025)

* fix(dashboard): custom css should be removed on unmount

* better comment

* remove unnecessary typecast

* move type to top level scope
---
 .../dashboard/containers/DashboardPage.tsx    | 40 ++++++++-----------
 ...{injectCustomCss.js => injectCustomCss.ts} | 36 ++++++++++++-----
 2 files changed, 43 insertions(+), 33 deletions(-)
 rename superset-frontend/src/dashboard/util/{injectCustomCss.js => injectCustomCss.ts} (66%)

diff --git a/superset-frontend/src/dashboard/containers/DashboardPage.tsx b/superset-frontend/src/dashboard/containers/DashboardPage.tsx
index 580df62b3..e5897d18c 100644
--- a/superset-frontend/src/dashboard/containers/DashboardPage.tsx
+++ b/superset-frontend/src/dashboard/containers/DashboardPage.tsx
@@ -26,7 +26,6 @@ import {
   useDashboardDatasets,
 } from 'src/common/hooks/apiResources';
 import { ResourceStatus } from 'src/common/hooks/apiResources/apiResources';
-import { usePrevious } from 'src/common/hooks/usePrevious';
 import { hydrateDashboard } from 'src/dashboard/actions/hydrate';
 import injectCustomCss from 'src/dashboard/util/injectCustomCss';
 
@@ -42,14 +41,10 @@ const DashboardContainer = React.lazy(
 const DashboardPage: FC = () => {
   const dispatch = useDispatch();
   const { idOrSlug } = useParams<{ idOrSlug: string }>();
-  const [isLoaded, setLoaded] = useState(false);
+  const [isHydrated, setHydrated] = useState(false);
   const dashboardResource = useDashboard(idOrSlug);
   const chartsResource = useDashboardCharts(idOrSlug);
   const datasetsResource = useDashboardDatasets(idOrSlug);
-  const isLoading = [dashboardResource, chartsResource, datasetsResource].some(
-    resource => resource.status === ResourceStatus.LOADING,
-  );
-  const wasLoading = usePrevious(isLoading);
   const error = [dashboardResource, chartsResource, datasetsResource].find(
     resource => resource.status === ResourceStatus.ERROR,
   )?.error;
@@ -57,16 +52,22 @@ const DashboardPage: FC = () => {
   useEffect(() => {
     if (dashboardResource.result) {
       document.title = dashboardResource.result.dashboard_title;
+      if (dashboardResource.result.css) {
+        // returning will clean up custom css
+        // when dashboard unmounts or changes
+        return injectCustomCss(dashboardResource.result.css);
+      }
     }
+    return () => {};
   }, [dashboardResource.result]);
 
+  const shouldBeHydrated =
+    dashboardResource.status === ResourceStatus.COMPLETE &&
+    chartsResource.status === ResourceStatus.COMPLETE &&
+    datasetsResource.status === ResourceStatus.COMPLETE;
+
   useEffect(() => {
-    if (
-      wasLoading &&
-      dashboardResource.status === ResourceStatus.COMPLETE &&
-      chartsResource.status === ResourceStatus.COMPLETE &&
-      datasetsResource.status === ResourceStatus.COMPLETE
-    ) {
+    if (shouldBeHydrated) {
       dispatch(
         hydrateDashboard(
           dashboardResource.result,
@@ -74,20 +75,13 @@ const DashboardPage: FC = () => {
           datasetsResource.result,
         ),
       );
-      injectCustomCss(dashboardResource.result.css);
-      setLoaded(true);
+      setHydrated(true);
     }
-  }, [
-    dispatch,
-    wasLoading,
-    dashboardResource,
-    chartsResource,
-    datasetsResource,
-  ]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [shouldBeHydrated]);
 
   if (error) throw error; // caught in error boundary
-
-  if (!isLoaded) return <Loading />;
+  if (!isHydrated) return <Loading />;
   return <DashboardContainer />;
 };
 
diff --git a/superset-frontend/src/dashboard/util/injectCustomCss.js b/superset-frontend/src/dashboard/util/injectCustomCss.ts
similarity index 66%
rename from superset-frontend/src/dashboard/util/injectCustomCss.js
rename to superset-frontend/src/dashboard/util/injectCustomCss.ts
index 4f6238aad..b7db2b03d 100644
--- a/superset-frontend/src/dashboard/util/injectCustomCss.js
+++ b/superset-frontend/src/dashboard/util/injectCustomCss.ts
@@ -16,19 +16,31 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-export default function injectCustomCss(css) {
+
+function createStyleElement(className: string) {
+  const style = document.createElement('style');
+  style.className = className;
+  style.type = 'text/css';
+  return style;
+}
+
+// The original, non-typescript code referenced `style.styleSheet`.
+// I can't find what sort of element would have a styleSheet property,
+// so have created this type to satisfy TS without changing behavior.
+type MysteryStyleElement = {
+  styleSheet: {
+    cssText: string;
+  };
+};
+
+export default function injectCustomCss(css: string) {
   const className = 'CssEditor-css';
   const head = document.head || document.getElementsByTagName('head')[0];
-  let style = document.querySelector(`.${className}`);
+  const style: HTMLStyleElement =
+    document.querySelector(`.${className}`) || createStyleElement(className);
 
-  if (!style) {
-    style = document.createElement('style');
-    style.className = className;
-    style.type = 'text/css';
-  }
-
-  if (style.styleSheet) {
-    style.styleSheet.cssText = css;
+  if ('styleSheet' in style) {
+    (style as MysteryStyleElement).styleSheet.cssText = css;
   } else {
     style.innerHTML = css;
   }
@@ -45,4 +57,8 @@ export default function injectCustomCss(css) {
    */
 
   head.appendChild(style);
+
+  return function removeCustomCSS() {
+    style.remove();
+  };
 }