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

@uppy/provider-views: migrate to TS #4919

Merged
merged 21 commits into from
Feb 27, 2024
Merged

@uppy/provider-views: migrate to TS #4919

merged 21 commits into from
Feb 27, 2024

Conversation

Murderlon
Copy link
Member

@Murderlon Murderlon commented Feb 14, 2024

This one was painfully tricky.

Separated plugins

provider-views and companion-client always belong together and operate on each other, but they are separate plugins, and not direct dependencies of each other. Most code just references code of the other plugin, but we can't use types from the other plugin as we don't have access to it.

To prevent incorrect types or introducing a breaking change by merging the packages, I deemed the best approach to duplicate some of the types into utils (CompanionClientProvider, CompanionFile) and create UnknownProviderPlugin and UnknownSearchProviderPlugin in core.

It's duplicate, but type safe! For instance, Inside companion-client the Provider class implements CompanionClientProvider. Any mismatch in types will be caught on either side. The UnknownProviderPlugin type is actually the single source of truth, as it's imported in Provider and used for getPlugin.

Faulty inheritance

Another place of needless complexity, is the View base class, which is used for ProviderView and SearchProviderView (that was my abstraction 😬🔫 ). View sets the provider property on the class, but that's different for ProviderView and SearchProviderView. Some type trickery is needed for this.

The "tag file"

In Uppy we have UppyFile, CompanionFile (introduced here), and Tagfile. The last one no one understands why it's there (as seen from an existing todo comment). Having this third type going around the codebase started posing problems, as now Restricter and some methods on Uppy now needed to deal with this.

For Restricter I solved this with yet another type: ValidateableFile. There had to be a type to indicate the minimal amount of properties present in order to validate it, then all file types can have overlap with it. Unfortunately RestrictionError has to maintain UppyFile, so only in Restricter a cast from ValidateableFile to UppyFile is required to please TS. Feel free to try locally to improve this.


In conclusion, this plugin is screaming to be rewritten.

@Murderlon Murderlon self-assigned this Feb 14, 2024
Copy link
Contributor

github-actions bot commented Feb 14, 2024

Diff output files
diff --git a/packages/@uppy/provider-views/lib/Breadcrumbs.js b/packages/@uppy/provider-views/lib/Breadcrumbs.js
index 0c5c87a..8364b3e 100644
--- a/packages/@uppy/provider-views/lib/Breadcrumbs.js
+++ b/packages/@uppy/provider-views/lib/Breadcrumbs.js
@@ -16,7 +16,7 @@ const Breadcrumb = props => {
     !isLast ? " / " : "",
   );
 };
-export default (props => {
+export default function Breadcrumbs(props) {
   const {
     getFolder,
     title,
@@ -34,10 +34,10 @@ export default (props => {
     breadcrumbs.map((directory, i) =>
       h(Breadcrumb, {
         key: directory.id,
-        getFolder: () => getFolder(directory.requestPath),
+        getFolder: () => getFolder(directory.requestPath, directory.name),
         title: i === 0 ? title : directory.name,
         isLast: i + 1 === breadcrumbs.length,
       })
     ),
   );
-});
+}
diff --git a/packages/@uppy/provider-views/lib/Browser.js b/packages/@uppy/provider-views/lib/Browser.js
index 87882d2..a3286fa 100644
--- a/packages/@uppy/provider-views/lib/Browser.js
+++ b/packages/@uppy/provider-views/lib/Browser.js
@@ -19,13 +19,10 @@ function ListItem(props) {
     i18n,
     validateRestrictions,
     getNextFolder,
-    columns,
     f,
   } = props;
   if (f.isFolder) {
-    var _isChecked;
     return Item({
-      columns,
       showTitles,
       viewType,
       i18n,
@@ -36,7 +33,7 @@ function ListItem(props) {
       toggleCheckbox: event => toggleCheckbox(event, f),
       recordShiftKeyPress,
       type: "folder",
-      isDisabled: (_isChecked = isChecked(f)) == null ? void 0 : _isChecked.loading,
+      isDisabled: false,
       isCheckboxDisabled: f.id === VIRTUAL_SHARED_DIR,
       handleFolderClick: () => getNextFolder(f),
     });
@@ -49,13 +46,13 @@ function ListItem(props) {
     getItemIcon: () => f.icon,
     isChecked: isChecked(f),
     toggleCheckbox: event => toggleCheckbox(event, f),
+    isCheckboxDisabled: false,
     recordShiftKeyPress,
-    columns,
     showTitles,
     viewType,
     i18n,
     type: "file",
-    isDisabled: restrictionError && !isChecked(f),
+    isDisabled: Boolean(restrictionError) && !isChecked(f),
     restrictionError,
   });
 }
@@ -86,7 +83,6 @@ function Browser(props) {
     getNextFolder,
     cancel,
     done,
-    columns,
     noResultsLabel,
     loadAllFiles,
   } = props;
@@ -160,7 +156,6 @@ function Browser(props) {
                   i18n: i18n,
                   validateRestrictions: validateRestrictions,
                   getNextFolder: getNextFolder,
-                  columns: columns,
                   f: f,
                 }),
               rowHeight: 31,
@@ -179,7 +174,7 @@ function Browser(props) {
             className: "uppy-ProviderBrowser-list",
             onScroll: handleScroll,
             role: "listbox",
-            tabIndex: "-1",
+            tabIndex: -1,
           },
           rows.map(f =>
             h(ListItem, {
@@ -193,7 +188,6 @@ function Browser(props) {
               i18n: i18n,
               validateRestrictions: validateRestrictions,
               getNextFolder: getNextFolder,
-              columns: columns,
               f: f,
             })
           ),
diff --git a/packages/@uppy/provider-views/lib/FooterActions.js b/packages/@uppy/provider-views/lib/FooterActions.js
index a2be68b..a57241b 100644
--- a/packages/@uppy/provider-views/lib/FooterActions.js
+++ b/packages/@uppy/provider-views/lib/FooterActions.js
@@ -1,5 +1,5 @@
 import { h } from "preact";
-export default (_ref => {
+export default function FooterActions(_ref) {
   let {
     cancel,
     done,
@@ -28,4 +28,4 @@ export default (_ref => {
       type: "button",
     }, i18n("cancel")),
   );
-});
+}
diff --git a/packages/@uppy/provider-views/lib/Item/components/GridLi.js b/packages/@uppy/provider-views/lib/Item/components/GridLi.js
index fde768b..ad7f93a 100644
--- a/packages/@uppy/provider-views/lib/Item/components/GridLi.js
+++ b/packages/@uppy/provider-views/lib/Item/components/GridLi.js
@@ -26,7 +26,7 @@ function GridListItem(props) {
     "li",
     {
       className: className,
-      title: isDisabled ? restrictionError == null ? void 0 : restrictionError.message : null,
+      title: isDisabled ? restrictionError == null ? void 0 : restrictionError.message : undefined,
     },
     h("input", {
       type: "checkbox",
diff --git a/packages/@uppy/provider-views/lib/Item/components/ItemIcon.js b/packages/@uppy/provider-views/lib/Item/components/ItemIcon.js
index 3d5766a..f101814 100644
--- a/packages/@uppy/provider-views/lib/Item/components/ItemIcon.js
+++ b/packages/@uppy/provider-views/lib/Item/components/ItemIcon.js
@@ -53,11 +53,11 @@ function VideoIcon() {
     }),
   );
 }
-export default (props => {
+export default function ItemIcon(props) {
   const {
     itemIconString,
   } = props;
-  if (itemIconString === null) return undefined;
+  if (itemIconString === null) return null;
   switch (itemIconString) {
     case "file":
       return h(FileIcon, null);
@@ -79,4 +79,4 @@ export default (props => {
       });
     }
   }
-});
+}
diff --git a/packages/@uppy/provider-views/lib/Item/components/ListLi.js b/packages/@uppy/provider-views/lib/Item/components/ListLi.js
index eff38c8..8784707 100644
--- a/packages/@uppy/provider-views/lib/Item/components/ListLi.js
+++ b/packages/@uppy/provider-views/lib/Item/components/ListLi.js
@@ -1,5 +1,5 @@
 import { h } from "preact";
-function ListItem(props) {
+export default function ListItem(props) {
   const {
     className,
     isDisabled,
@@ -20,7 +20,7 @@ function ListItem(props) {
     "li",
     {
       className: className,
-      title: isDisabled ? restrictionError == null ? void 0 : restrictionError.message : null,
+      title: isDisabled ? restrictionError == null ? void 0 : restrictionError.message : undefined,
     },
     !isCheckboxDisabled
       ? h("input", {
@@ -70,4 +70,3 @@ function ListItem(props) {
       ),
   );
 }
-export default ListItem;
diff --git a/packages/@uppy/provider-views/lib/Item/index.js b/packages/@uppy/provider-views/lib/Item/index.js
index 21caf1e..e7e4478 100644
--- a/packages/@uppy/provider-views/lib/Item/index.js
+++ b/packages/@uppy/provider-views/lib/Item/index.js
@@ -13,7 +13,7 @@ import { h } from "preact";
 import GridListItem from "./components/GridLi.js";
 import ItemIcon from "./components/ItemIcon.js";
 import ListItem from "./components/ListLi.js";
-export default (props => {
+export default function Item(props) {
   const {
     author,
     getItemIcon,
@@ -61,10 +61,10 @@ export default (props => {
           target: "_blank",
           rel: "noopener noreferrer",
           className: "uppy-ProviderBrowserItem-author",
-          tabIndex: "-1",
+          tabIndex: -1,
         }, author.name),
       );
     default:
       throw new Error(`There is no such type ${viewType}`);
   }
-});
+}
diff --git a/packages/@uppy/provider-views/lib/Loader.js b/packages/@uppy/provider-views/lib/Loader.js
index 950d808..28a4f59 100644
--- a/packages/@uppy/provider-views/lib/Loader.js
+++ b/packages/@uppy/provider-views/lib/Loader.js
@@ -1,5 +1,5 @@
 import { h } from "preact";
-export default (_ref => {
+export default function Loader(_ref) {
   let {
     i18n,
     loading,
@@ -16,4 +16,4 @@ export default (_ref => {
       },
     }, loading),
   );
-});
+}
diff --git a/packages/@uppy/provider-views/lib/ProviderView/AuthView.js b/packages/@uppy/provider-views/lib/ProviderView/AuthView.js
index 6b43993..3902d4f 100644
--- a/packages/@uppy/provider-views/lib/ProviderView/AuthView.js
+++ b/packages/@uppy/provider-views/lib/ProviderView/AuthView.js
@@ -47,7 +47,7 @@ function GoogleIcon() {
     ),
   );
 }
-const DefaultForm = _ref => {
+function DefaultForm(_ref) {
   let {
     pluginName,
     i18n,
@@ -86,7 +86,7 @@ const DefaultForm = _ref => {
         }),
       ),
   );
-};
+}
 const defaultRenderForm = _ref2 => {
   let {
     pluginName,
@@ -99,7 +99,7 @@ const defaultRenderForm = _ref2 => {
     onAuth: onAuth,
   });
 };
-function AuthView(props) {
+export default function AuthView(props) {
   const {
     loading,
     pluginName,
@@ -108,14 +108,6 @@ function AuthView(props) {
     handleAuth,
     renderForm = defaultRenderForm,
   } = props;
-  const pluginNameComponent = h(
-    "span",
-    {
-      className: "uppy-Provider-authTitleName",
-    },
-    pluginName,
-    h("br", null),
-  );
   return h(
     "div",
     {
@@ -130,7 +122,7 @@ function AuthView(props) {
         className: "uppy-Provider-authTitle",
       },
       i18n("authenticateWithTitle", {
-        pluginName: pluginNameComponent,
+        pluginName,
       }),
     ),
     h(
@@ -147,4 +139,3 @@ function AuthView(props) {
     ),
   );
 }
-export default AuthView;
diff --git a/packages/@uppy/provider-views/lib/ProviderView/Header.js b/packages/@uppy/provider-views/lib/ProviderView/Header.js
index f08043a..b0aa7bc 100644
--- a/packages/@uppy/provider-views/lib/ProviderView/Header.js
+++ b/packages/@uppy/provider-views/lib/ProviderView/Header.js
@@ -1,19 +1,20 @@
+import { Fragment, h } from "preact";
 import Breadcrumbs from "../Breadcrumbs.js";
 import User from "./User.js";
-export default (props => {
-  const components = [];
-  if (props.showBreadcrumbs) {
-    components.push(Breadcrumbs({
+export default function Header(props) {
+  return h(
+    Fragment,
+    null,
+    props.showBreadcrumbs && h(Breadcrumbs, {
       getFolder: props.getFolder,
       breadcrumbs: props.breadcrumbs,
       breadcrumbsIcon: props.pluginIcon && props.pluginIcon(),
       title: props.title,
-    }));
-  }
-  components.push(User({
-    logout: props.logout,
-    username: props.username,
-    i18n: props.i18n,
-  }));
-  return components;
-});
+    }),
+    h(User, {
+      logout: props.logout,
+      username: props.username,
+      i18n: props.i18n,
+    }),
+  );
+}
diff --git a/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js b/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js
index 38d3450..98fa377 100644
--- a/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js
+++ b/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js
@@ -41,6 +41,13 @@ export function defaultPickerIcon() {
     }),
   );
 }
+const defaultOptions = {
+  viewType: "list",
+  showTitles: true,
+  showFilter: true,
+  showBreadcrumbs: true,
+  loadAllFiles: false,
+};
 var _abortController = _classPrivateFieldLooseKey("abortController");
 var _withAbort = _classPrivateFieldLooseKey("withAbort");
 var _list = _classPrivateFieldLooseKey("list");
@@ -48,7 +55,10 @@ var _listFilesAndFolders = _classPrivateFieldLooseKey("listFilesAndFolders");
 var _recursivelyListAllFiles = _classPrivateFieldLooseKey("recursivelyListAllFiles");
 export default class ProviderView extends View {
   constructor(plugin, opts) {
-    super(plugin, opts);
+    super(plugin, {
+      ...defaultOptions,
+      ...opts,
+    });
     Object.defineProperty(this, _recursivelyListAllFiles, {
       value: _recursivelyListAllFiles2,
     });
@@ -65,17 +75,6 @@ export default class ProviderView extends View {
       writable: true,
       value: void 0,
     });
-    const defaultOptions = {
-      viewType: "list",
-      showTitles: true,
-      showFilter: true,
-      showBreadcrumbs: true,
-      loadAllFiles: false,
-    };
-    this.opts = {
-      ...defaultOptions,
-      ...opts,
-    };
     this.filterQuery = this.filterQuery.bind(this);
     this.clearFilter = this.clearFilter.bind(this);
     this.getFolder = this.getFolder.bind(this);
@@ -400,7 +399,7 @@ export default class ProviderView extends View {
       handleScroll: this.handleScroll,
       done: this.donePicking,
       cancel: this.cancelPicking,
-      headerComponent: Header(headerProps),
+      headerComponent: h(Header, headerProps),
       title: this.plugin.title,
       viewType: targetViewOptions.viewType,
       showTitles: targetViewOptions.showTitles,
diff --git a/packages/@uppy/provider-views/lib/ProviderView/User.js b/packages/@uppy/provider-views/lib/ProviderView/User.js
index 874d30a..eff0033 100644
--- a/packages/@uppy/provider-views/lib/ProviderView/User.js
+++ b/packages/@uppy/provider-views/lib/ProviderView/User.js
@@ -1,11 +1,13 @@
-import { h } from "preact";
-export default (_ref => {
+import { Fragment, h } from "preact";
+export default function User(_ref) {
   let {
     i18n,
     logout,
     username,
   } = _ref;
-  return [
+  return h(
+    Fragment,
+    null,
     h("span", {
       className: "uppy-ProviderBrowser-user",
       key: "username",
@@ -16,5 +18,5 @@ export default (_ref => {
       className: "uppy-u-reset uppy-c-btn uppy-ProviderBrowser-userLogout",
       key: "logout",
     }, i18n("logOut")),
-  ];
-});
+  );
+}
diff --git a/packages/@uppy/provider-views/lib/SearchFilterInput.js b/packages/@uppy/provider-views/lib/SearchFilterInput.js
index 247622f..e358931 100644
--- a/packages/@uppy/provider-views/lib/SearchFilterInput.js
+++ b/packages/@uppy/provider-views/lib/SearchFilterInput.js
@@ -60,7 +60,7 @@ export default function SearchFilterInput(props) {
       {
         "aria-hidden": "true",
         focusable: "false",
-        class: "uppy-c-icon uppy-ProviderBrowser-searchFilterIcon",
+        className: "uppy-c-icon uppy-ProviderBrowser-searchFilterIcon",
         width: "12",
         height: "12",
         viewBox: "0 0 12 12",
diff --git a/packages/@uppy/provider-views/lib/SearchProviderView/SearchProviderView.js b/packages/@uppy/provider-views/lib/SearchProviderView/SearchProviderView.js
index 0b34ac3..284a74b 100644
--- a/packages/@uppy/provider-views/lib/SearchProviderView/SearchProviderView.js
+++ b/packages/@uppy/provider-views/lib/SearchProviderView/SearchProviderView.js
@@ -16,44 +16,44 @@ import View from "../View.js";
 const packageJson = {
   "version": "3.9.1",
 };
+const defaultState = {
+  isInputMode: true,
+  files: [],
+  folders: [],
+  breadcrumbs: [],
+  filterInput: "",
+  currentSelection: [],
+  searchTerm: null,
+};
+const defaultOptions = {
+  viewType: "grid",
+  showTitles: true,
+  showFilter: true,
+  showBreadcrumbs: true,
+};
 var _updateFilesAndInputMode = _classPrivateFieldLooseKey("updateFilesAndInputMode");
 export default class SearchProviderView extends View {
   constructor(plugin, opts) {
-    super(plugin, opts);
+    super(plugin, {
+      ...defaultOptions,
+      ...opts,
+    });
     Object.defineProperty(this, _updateFilesAndInputMode, {
       value: _updateFilesAndInputMode2,
     });
-    const defaultOptions = {
-      viewType: "grid",
-      showTitles: false,
-      showFilter: false,
-      showBreadcrumbs: false,
-    };
-    this.opts = {
-      ...defaultOptions,
-      ...opts,
-    };
+    this.nextPageQuery = null;
     this.search = this.search.bind(this);
     this.clearSearch = this.clearSearch.bind(this);
     this.resetPluginState = this.resetPluginState.bind(this);
     this.handleScroll = this.handleScroll.bind(this);
     this.donePicking = this.donePicking.bind(this);
     this.render = this.render.bind(this);
-    this.defaultState = {
-      isInputMode: true,
-      files: [],
-      folders: [],
-      breadcrumbs: [],
-      filterInput: "",
-      currentSelection: [],
-      searchTerm: null,
-    };
-    this.plugin.setPluginState(this.defaultState);
+    this.plugin.setPluginState(defaultState);
     this.registerRequestClient();
   }
   tearDown() {}
   resetPluginState() {
-    this.plugin.setPluginState(this.defaultState);
+    this.plugin.setPluginState(defaultState);
   }
   async search(query) {
     const {
@@ -183,7 +183,6 @@ export default class SearchProviderView extends View {
           },
           h(SearchFilterInput, {
             search: this.search,
-            clearSelection: this.clearSelection,
             inputLabel: i18n("enterTextToSearch"),
             buttonLabel: i18n("searchImages"),
             inputClassName: "uppy-c-textInput uppy-SearchProvider-input",
diff --git a/packages/@uppy/provider-views/lib/View.js b/packages/@uppy/provider-views/lib/View.js
index ae5f90f..782b4c5 100644
--- a/packages/@uppy/provider-views/lib/View.js
+++ b/packages/@uppy/provider-views/lib/View.js
@@ -81,6 +81,7 @@ export default class View {
     };
     this.plugin = plugin;
     this.provider = opts.provider;
+    this.opts = opts;
     this.isHandlingScroll = false;
     this.preFirstRender = this.preFirstRender.bind(this);
     this.handleError = this.handleError.bind(this);
@@ -142,10 +143,10 @@ export default class View {
     const tagFile = {
       id: file.id,
       source: this.plugin.id,
-      data: file,
       name: file.name || file.id,
       type: file.mimeType,
       isRemote: true,
+      data: file,
       meta: {},
       body: {
         fileId: file.id,

* main:
  Image editor: make compressor work after the image editor, too (#4918)
  meta: exclude `tsconfig` files from npm bundles (#4916)
  @uppy/compressor: migrate to TS (#4907)
  Update uppy-ProviderBrowser-viewType--list.scss (#4913)
  @uppy/tus: migrate to TS (#4899)
* main:
  @uppy/file-input: refactor to TypeScript (#4954)
Copy link

socket-security bot commented Feb 26, 2024

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@Murderlon Murderlon merged commit a4bbd82 into main Feb 27, 2024
16 checks passed
@Murderlon Murderlon deleted the provider-views-ts branch February 27, 2024 17:15
@github-actions github-actions bot mentioned this pull request Feb 28, 2024
github-actions bot added a commit that referenced this pull request Feb 28, 2024
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/box              |   2.2.1 | @uppy/onedrive         |   3.2.1 |
| @uppy/companion-client |   3.7.4 | @uppy/progress-bar     |   3.1.0 |
| @uppy/core             |   3.9.3 | @uppy/provider-views   |  3.10.0 |
| @uppy/dashboard        |   3.7.5 | @uppy/status-bar       |   3.3.0 |
| @uppy/file-input       |   3.1.0 | @uppy/utils            |   5.7.4 |
| @uppy/form             |   3.2.0 | @uppy/xhr-upload       |   3.6.4 |
| @uppy/image-editor     |   2.4.4 | uppy                   |  3.23.0 |
| @uppy/informer         |   3.1.0 |                        |         |

- @uppy/form: migrate to TS (Merlijn Vos / #4937)
- @uppy/box: fetchPreAuthToken in box too (Mikael Finstad / #4969)
- @uppy/progress-bar: refactor to TypeScript (Mikael Finstad / #4921)
- @uppy/onedrive: fix custom oauth2 credentials for onedrive (Mikael Finstad / #4968)
- @uppy/companion-client,@uppy/utils,@uppy/xhr-upload: improvements for #4922 (Mikael Finstad / #4960)
- @uppy/utils: fix various type issues (Mikael Finstad / #4958)
- @uppy/provider-views: migrate to TS (Merlijn Vos / #4919)
- @uppy/utils: simplify `findDOMElements` (Mikael Finstad / #4957)
- @uppy/xhr-upload: fix getResponseData regression (Merlijn Vos / #4964)
- @uppy/informer: migrate to TS (Merlijn Vos / #4967)
- @uppy/core: remove unused import (Antoine du Hamel / #4972)
- @uppy/image-editor: remove default target (Merlijn Vos / #4966)
- @uppy/angular: Build fixes (Mikael Finstad / #4959)
- meta: Fix flaky e2e test (Murderlon)
- meta: fix e2e flake (Mikael Finstad / #4961)
- meta: add support for `Fragment` short syntax (Antoine du Hamel / #4953)
- @uppy/file-input: refactor to TypeScript (Antoine du Hamel / #4954)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants