fix super.toggleCheckbox bug

[mifi]

this includes a new check which uncovers a bug with super.toggleCheckbox:

Error: packages/@uppy/google-drive/src/DriveProviderViews.ts(15,13): error TS2855: Class field 'toggleCheckbox' defined by the parent class is not accessible in the child class via super.

https://github.com/transloadit/uppy/actions/runs/8315808580/job/22754550095?pr=5004

TIL instance methods method() {} are accessible from subclasses via super, but instance fields are not! method = () => {} (thanks typescript!)

Also, currently main is broken for me when running in dev: the checkboxes for google drive don't do anything and there's an error about toggleCheckbox not being a method.

Edit: So I now also pushed a commit to fix this bug which apparently was introduced on apr 4 2023 in this commit 3dd1e5e when the method was converted into a field.

Why it currently works on https://uppy.io/examples/ (and apperently has been working since apr 2023) is beyond me.

[github-actions]

Diff output files

diff --git a/packages/@uppy/google-drive/lib/DriveProviderViews.js b/packages/@uppy/google-drive/lib/DriveProviderViews.js
index 3343fdc..4ac63e3 100644
--- a/packages/@uppy/google-drive/lib/DriveProviderViews.js
+++ b/packages/@uppy/google-drive/lib/DriveProviderViews.js
@@ -1,13 +1,10 @@
 import { ProviderViews } from "@uppy/provider-views";
 export default class DriveProviderViews extends ProviderViews {
-  constructor() {
-    super(...arguments);
-    this.toggleCheckbox = (e, file) => {
-      e.stopPropagation();
-      e.preventDefault();
-      if (!file.custom.isSharedDrive) {
-        super.toggleCheckbox(e, file);
-      }
-    };
+  toggleCheckbox(e, file) {
+    e.stopPropagation();
+    e.preventDefault();
+    if (!file.custom.isSharedDrive) {
+      super.toggleCheckbox(e, file);
+    }
   }
 }
diff --git a/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js b/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js
index aa49d22..83e1cd9 100644
--- a/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js
+++ b/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js
@@ -361,7 +361,6 @@ export default class ProviderView extends View {
     } = this.plugin.getPluginState();
     const {
       isChecked,
-      toggleCheckbox,
       recordShiftKeyPress,
       filterItems,
     } = this;
@@ -379,7 +378,7 @@ export default class ProviderView extends View {
     };
     const browserProps = {
       isChecked,
-      toggleCheckbox,
+      toggleCheckbox: this.toggleCheckbox.bind(this),
       recordShiftKeyPress,
       currentSelection,
       files: hasInput ? filterItems(files) : files,
diff --git a/packages/@uppy/provider-views/lib/SearchProviderView/SearchProviderView.js b/packages/@uppy/provider-views/lib/SearchProviderView/SearchProviderView.js
index ed4dea4..e5ff004 100644
--- a/packages/@uppy/provider-views/lib/SearchProviderView/SearchProviderView.js
+++ b/packages/@uppy/provider-views/lib/SearchProviderView/SearchProviderView.js
@@ -134,14 +134,13 @@ export default class SearchProviderView extends View {
     } = this.plugin.getPluginState();
     const {
       isChecked,
-      toggleCheckbox,
       filterItems,
       recordShiftKeyPress,
     } = this;
     const hasInput = filterInput !== "";
     const browserProps = {
       isChecked,
-      toggleCheckbox,
+      toggleCheckbox: this.toggleCheckbox.bind(this),
       recordShiftKeyPress,
       currentSelection,
       files: hasInput ? filterItems(files) : files,
diff --git a/packages/@uppy/provider-views/lib/View.js b/packages/@uppy/provider-views/lib/View.js
index 782b4c5..69cd639 100644
--- a/packages/@uppy/provider-views/lib/View.js
+++ b/packages/@uppy/provider-views/lib/View.js
@@ -15,64 +15,6 @@ export default class View {
     this.recordShiftKeyPress = e => {
       this.isShiftKeyPressed = e.shiftKey;
     };
-    this.toggleCheckbox = (e, file) => {
-      e.stopPropagation();
-      e.preventDefault();
-      e.currentTarget.focus();
-      const {
-        folders,
-        files,
-      } = this.plugin.getPluginState();
-      const items = this.filterItems(folders.concat(files));
-      if (this.lastCheckbox && this.isShiftKeyPressed) {
-        const {
-          currentSelection,
-        } = this.plugin.getPluginState();
-        const prevIndex = items.indexOf(this.lastCheckbox);
-        const currentIndex = items.indexOf(file);
-        const newSelection = prevIndex < currentIndex
-          ? items.slice(prevIndex, currentIndex + 1)
-          : items.slice(currentIndex, prevIndex + 1);
-        const reducedNewSelection = [];
-        for (const item of newSelection) {
-          const {
-            uppy,
-          } = this.plugin;
-          const restrictionError = uppy.validateRestrictions(remoteFileObjToLocal(item), [
-            ...uppy.getFiles(),
-            ...reducedNewSelection,
-          ]);
-          if (!restrictionError) {
-            reducedNewSelection.push(item);
-          } else {
-            uppy.info(
-              {
-                message: restrictionError.message,
-              },
-              "error",
-              uppy.opts.infoTimeout,
-            );
-          }
-        }
-        this.plugin.setPluginState({
-          currentSelection: [...new Set([...currentSelection, ...reducedNewSelection])],
-        });
-        return;
-      }
-      this.lastCheckbox = file;
-      const {
-        currentSelection,
-      } = this.plugin.getPluginState();
-      if (this.isChecked(file)) {
-        this.plugin.setPluginState({
-          currentSelection: currentSelection.filter(item => item.id !== file.id),
-        });
-      } else {
-        this.plugin.setPluginState({
-          currentSelection: currentSelection.concat([file]),
-        });
-      }
-    };
     this.isChecked = file => {
       const {
         currentSelection,
@@ -182,6 +124,64 @@ export default class View {
     }
     return tagFile;
   }
+  toggleCheckbox(e, file) {
+    e.stopPropagation();
+    e.preventDefault();
+    e.currentTarget.focus();
+    const {
+      folders,
+      files,
+    } = this.plugin.getPluginState();
+    const items = this.filterItems(folders.concat(files));
+    if (this.lastCheckbox && this.isShiftKeyPressed) {
+      const {
+        currentSelection,
+      } = this.plugin.getPluginState();
+      const prevIndex = items.indexOf(this.lastCheckbox);
+      const currentIndex = items.indexOf(file);
+      const newSelection = prevIndex < currentIndex
+        ? items.slice(prevIndex, currentIndex + 1)
+        : items.slice(currentIndex, prevIndex + 1);
+      const reducedNewSelection = [];
+      for (const item of newSelection) {
+        const {
+          uppy,
+        } = this.plugin;
+        const restrictionError = uppy.validateRestrictions(remoteFileObjToLocal(item), [
+          ...uppy.getFiles(),
+          ...reducedNewSelection,
+        ]);
+        if (!restrictionError) {
+          reducedNewSelection.push(item);
+        } else {
+          uppy.info(
+            {
+              message: restrictionError.message,
+            },
+            "error",
+            uppy.opts.infoTimeout,
+          );
+        }
+      }
+      this.plugin.setPluginState({
+        currentSelection: [...new Set([...currentSelection, ...reducedNewSelection])],
+      });
+      return;
+    }
+    this.lastCheckbox = file;
+    const {
+      currentSelection,
+    } = this.plugin.getPluginState();
+    if (this.isChecked(file)) {
+      this.plugin.setPluginState({
+        currentSelection: currentSelection.filter(item => item.id !== file.id),
+      });
+    } else {
+      this.plugin.setPluginState({
+        currentSelection: currentSelection.concat([file]),
+      });
+    }
+  }
   setLoading(loading) {
     this.plugin.setPluginState({
       loading,

[aduh95]

Is the upgrade to TS 5.4 necessary? The version of Angular we're using is using 5.1, and using a single version seems preferable

[Murderlon]

Good catch

[Murderlon]

Do we want our e2e tests to share this? I don't think so, it's a very different setup.

[mifi]

I did this because the current e2e/tsconfig.json doesn't work with newest typescript. if not extend, shall we instead just copy-paste everything from tsconfig.shared.json into e2e/tsconfig.json and just make sure to manually keep them up to date in the future?

[Murderlon]

Only add the settings we need to add, not everything. Let's start minimal.

[mifi]

so we don't want complete/strict type checking for e2e?

[mifi]

Is the upgrade to TS 5.4 necessary? The version of Angular we're using is using 5.1, and using a single version seems preferable

seems like 5.2 is needed to have this check. maybe we can instead upgrade angular? I think keeping up to date with new typescript features is a good thing, no?

[aduh95]

Is the upgrade to TS 5.4 necessary? The version of Angular we're using is using 5.1, and using a single version seems preferable

seems like 5.2 is needed to have this check. maybe we can instead upgrade angular? I think keeping up to date with new typescript features is a good thing, no?

Of course, and it's on the roadmap, see #5008. However, given it's a breaking change, it will probably happen only on the 4.x branch. It seems to me we can land the fix without the TS upgrade, or we can wait for the Angular upgrade and land this afterwards. I'd prefer the former.

[mifi]

or a third option is we can merge this as-is and then later once we upgrade angular, we can also upgrade angular's own ts version? upgrading ts is not a breaking change afaik

[aduh95]

upgrading ts is not a breaking change afaik

It's not, I agree.

we can merge this as-is and then later once we upgrade angular, we can also upgrade angular's own ts version?

It seems to me preferable not to have several TS versions in the monorepo, I think we have more to lose than to gain. My preference would be to not land the TS upgrade with this PR.

[mifi]

ok i've now reverted all these changes

[aduh95]

So I guess the rule from now on is that only private properties are allowed to be function, otherwise we should use a method and call .bind(this)

[mifi]

I don't think we need any rule, because typescript will take care of the problem once we upgrade

[aduh95]

I don't think we need any rule, because typescript will take care of the problem once we upgrade

Obviously TS can't detect if consumers of our package call super., it's important that we keep in mind our plugins are not used only in this monorepo.

[mifi]

that's a good point. i wonder if eslint could enforce using only methods (unless private). couldn't find a rule