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

Google Picker #5443

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Google Picker #5443

wants to merge 22 commits into from

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Sep 2, 2024

Initial POC of Google Picker.

closes #5382
closes #5469

How to test:

Google Developer console setup

Instructions for enabling the Picker API in the Google Developer console for the first time:

Now set VITE_GOOGLE_PICKER_CLIENT_ID, VITE_GOOGLE_PICKER_API_KEY, VITE_GOOGLE_PICKER_APP_ID in your .env file and run yarn dev:with-companion

Problems/discussion

  • redirect_uri needs to be frontend URI, and wilcard is not allowed. Update: I've made appId, clientId and apiKey as Uppy options (not Companion) so this should be OK because people can then use their own credentials.
  • UI is not very good, wraps weirdly etc
  • No deep/multi select inside folders like we have now with Uppy
  • Cannot seem to be able to select drives shared with me
  • Cannot seem to get google photos to work (only Google Drive)
  • In the end I decided not to put it in RemoteSources because it has different options
  • Access token from Google is stored in Local Storage in the browser.

TODO

  • tsconfig.json (I have just copy-pasted from google photos)
  • tsconfig.build.json (I have just copy-pasted from google photos)
  • google-picker.mdx (I have just copy-pasted from google photos)
  • google-picker/README.md (I have just copy-pasted from google photos)
  • in onPicked add the files along with any metadata needed by companion to download the files, like accessToken, clientId etc.
  • Create a new endpoint in companion to allow downloading files and pass the files along with parameters like clientId, scope, accessToken. The endpoint will call gapi.client.drive.files.get, and stream (download/upload) the file to the destination (e.g. tus/s3 etc). Need to make sure the endpoint is secure somehow in a way that it cannot be abused/exploited.
  • maybe this.provider is not needed in GooglePicker.ts
  • Need to save the auth token somewhere? (local storage?)
  • handle folders and shared files
  • check mediaMetadata status field READY
  • not sure how to allow selecting folders (setSelectFolderEnabled(true))
  • baseUrls expire after 60 minutes (google photos), so we might have to refresh them if an upload takes very long
  • logout method on provider
  • do the same for google photos picker api
  • include more metadata (width,height,thumbnail?)

Copy link
Contributor

github-actions bot commented Sep 2, 2024

Diff output files
diff --git a/packages/@uppy/companion-client/lib/Provider.js b/packages/@uppy/companion-client/lib/Provider.js
index dfd371e..8be290a 100644
--- a/packages/@uppy/companion-client/lib/Provider.js
+++ b/packages/@uppy/companion-client/lib/Provider.js
@@ -228,7 +228,7 @@ export default class Provider extends RequestClient {
       if (_classPrivateFieldLooseBase(this, _refreshingTokenPromise)[_refreshingTokenPromise] == null) {
         _classPrivateFieldLooseBase(this, _refreshingTokenPromise)[_refreshingTokenPromise] = (async () => {
           try {
-            this.uppy.log(`[CompanionClient] Refreshing expired auth token`, "info");
+            this.uppy.log(`[CompanionClient] Refreshing expired auth token`);
             const response = await super.request({
               path: this.refreshTokenUrl(),
               method: "POST",
diff --git a/packages/@uppy/companion-client/lib/RequestClient.js b/packages/@uppy/companion-client/lib/RequestClient.js
index 70e2b15..3499c67 100644
--- a/packages/@uppy/companion-client/lib/RequestClient.js
+++ b/packages/@uppy/companion-client/lib/RequestClient.js
@@ -389,7 +389,7 @@ async function _awaitRemoteFileUpload2(_ref4) {
                   }
                 });
                 const closeSocket = () => {
-                  this.uppy.log(`Closing socket ${file.id}`, "info");
+                  this.uppy.log(`Closing socket ${file.id}`);
                   clearTimeout(activityTimeout);
                   if (socket) socket.close();
                   socket = undefined;
@@ -403,7 +403,7 @@ async function _awaitRemoteFileUpload2(_ref4) {
               signal: socketAbortController.signal,
               onFailedAttempt: () => {
                 if (socketAbortController.signal.aborted) return;
-                this.uppy.log(`Retrying websocket ${file.id}`, "info");
+                this.uppy.log(`Retrying websocket ${file.id}`);
               },
             });
           })().abortOn(socketAbortController.signal);
@@ -424,7 +424,7 @@ async function _awaitRemoteFileUpload2(_ref4) {
         socketSend("cancel");
         (_socketAbortControlle3 = socketAbortController) == null || _socketAbortControlle3.abort == null
           || _socketAbortControlle3.abort();
-        this.uppy.log(`upload ${file.id} was removed`, "info");
+        this.uppy.log(`upload ${file.id} was removed`);
         resolve();
       };
       const onCancelAll = () => {
@@ -432,7 +432,7 @@ async function _awaitRemoteFileUpload2(_ref4) {
         socketSend("cancel");
         (_socketAbortControlle4 = socketAbortController) == null || _socketAbortControlle4.abort == null
           || _socketAbortControlle4.abort();
-        this.uppy.log(`upload ${file.id} was canceled`, "info");
+        this.uppy.log(`upload ${file.id} was canceled`);
         resolve();
       };
       const onFilePausedChange = (targetFile, newPausedState) => {
diff --git a/packages/@uppy/companion-client/lib/tokenStorage.js b/packages/@uppy/companion-client/lib/tokenStorage.js
index df49432..26bb521 100644
--- a/packages/@uppy/companion-client/lib/tokenStorage.js
+++ b/packages/@uppy/companion-client/lib/tokenStorage.js
@@ -1,15 +1,9 @@
-export function setItem(key, value) {
-  return new Promise(resolve => {
-    localStorage.setItem(key, value);
-    resolve();
-  });
+export async function setItem(key, value) {
+  localStorage.setItem(key, value);
 }
-export function getItem(key) {
-  return Promise.resolve(localStorage.getItem(key));
+export async function getItem(key) {
+  return localStorage.getItem(key);
 }
-export function removeItem(key) {
-  return new Promise(resolve => {
-    localStorage.removeItem(key);
-    resolve();
-  });
+export async function removeItem(key) {
+  localStorage.removeItem(key);
 }
diff --git a/packages/@uppy/companion/lib/companion.js b/packages/@uppy/companion/lib/companion.js
index 9416e5e..333b8b9 100644
--- a/packages/@uppy/companion/lib/companion.js
+++ b/packages/@uppy/companion/lib/companion.js
@@ -11,6 +11,7 @@ const providerManager = require("./server/provider");
 const controllers = require("./server/controllers");
 const s3 = require("./server/controllers/s3");
 const url = require("./server/controllers/url");
+const googlePicker = require("./server/controllers/googlePicker");
 const createEmitter = require("./server/emitter");
 const redis = require("./server/redis");
 const jobs = require("./server/jobs");
@@ -107,6 +108,9 @@ module.exports.app = (optionsArg = {}) => {
   if (options.enableUrlEndpoint) {
     app.use("/url", url());
   }
+  if (options.enableGooglePickerEndpoint) {
+    app.use("/google-picker", googlePicker());
+  }
   app.post(
     "/:providerName/preauth",
     express.json(),
diff --git a/packages/@uppy/companion/lib/config/companion.d.ts b/packages/@uppy/companion/lib/config/companion.d.ts
index 3924a64..9aff10f 100644
--- a/packages/@uppy/companion/lib/config/companion.d.ts
+++ b/packages/@uppy/companion/lib/config/companion.d.ts
@@ -12,6 +12,7 @@ export namespace defaultOptions {
         export let expires: number;
     }
     let enableUrlEndpoint: boolean;
+    let enableGooglePickerEndpoint: boolean;
     let allowLocalUrls: boolean;
     let periodicPingUrls: any[];
     let streamingUpload: boolean;
diff --git a/packages/@uppy/companion/lib/config/companion.js b/packages/@uppy/companion/lib/config/companion.js
index 542f378..5892411 100644
--- a/packages/@uppy/companion/lib/config/companion.js
+++ b/packages/@uppy/companion/lib/config/companion.js
@@ -18,6 +18,7 @@ const defaultOptions = {
     expires: 800, // seconds
   },
   enableUrlEndpoint: false,
+  enableGooglePickerEndpoint: false,
   allowLocalUrls: false,
   periodicPingUrls: [],
   streamingUpload: true,
diff --git a/packages/@uppy/companion/lib/server/controllers/url.js b/packages/@uppy/companion/lib/server/controllers/url.js
index 3e0eff3..6a0b3be 100644
--- a/packages/@uppy/companion/lib/server/controllers/url.js
+++ b/packages/@uppy/companion/lib/server/controllers/url.js
@@ -2,35 +2,15 @@
 Object.defineProperty(exports, "__esModule", { value: true });
 const express = require("express");
 const { startDownUpload } = require("../helpers/upload");
-const { prepareStream } = require("../helpers/utils");
+const { downloadURL } = require("../download");
 const { validateURL } = require("../helpers/request");
-const { getURLMeta, getProtectedGot } = require("../helpers/request");
+const { getURLMeta } = require("../helpers/request");
 const logger = require("../logger");
 /**
  * @callback downloadCallback
  * @param {Error} err
  * @param {string | Buffer | Buffer[]} chunk
  */
-/**
- * Downloads the content in the specified url, and passes the data
- * to the callback chunk by chunk.
- *
- * @param {string} url
- * @param {boolean} allowLocalIPs
- * @param {string} traceId
- * @returns {Promise}
- */
-const downloadURL = async (url, allowLocalIPs, traceId) => {
-  try {
-    const protectedGot = await getProtectedGot({ allowLocalIPs });
-    const stream = protectedGot.stream.get(url, { responseType: "json" });
-    const { size } = await prepareStream(stream);
-    return { stream, size };
-  } catch (err) {
-    logger.error(err, "controller.url.download.error", traceId);
-    throw err;
-  }
-};
 /**
  * Fetches the size and content type of a URL
  *
diff --git a/packages/@uppy/companion/lib/server/helpers/request.d.ts b/packages/@uppy/companion/lib/server/helpers/request.d.ts
index eff658a..69a7536 100644
--- a/packages/@uppy/companion/lib/server/helpers/request.d.ts
+++ b/packages/@uppy/companion/lib/server/helpers/request.d.ts
@@ -1,5 +1,5 @@
 /// <reference types="node" />
-export function getURLMeta(url: string, allowLocalIPs?: boolean): Promise<{
+export function getURLMeta(url: string, allowLocalIPs?: boolean, options?: any): Promise<{
     name: string;
     type: string;
     size: number;
diff --git a/packages/@uppy/companion/lib/server/helpers/request.js b/packages/@uppy/companion/lib/server/helpers/request.js
index 9c78f29..fc2bd1b 100644
--- a/packages/@uppy/companion/lib/server/helpers/request.js
+++ b/packages/@uppy/companion/lib/server/helpers/request.js
@@ -94,10 +94,10 @@ module.exports.getProtectedGot = getProtectedGot;
  * @param {boolean} allowLocalIPs
  * @returns {Promise<{name: string, type: string, size: number}>}
  */
-exports.getURLMeta = async (url, allowLocalIPs = false) => {
+exports.getURLMeta = async (url, allowLocalIPs = false, options = undefined) => {
   async function requestWithMethod(method) {
     const protectedGot = await getProtectedGot({ allowLocalIPs });
-    const stream = protectedGot.stream(url, { method, throwHttpErrors: false });
+    const stream = protectedGot.stream(url, { method, throwHttpErrors: false, ...options });
     return new Promise((resolve, reject) => (stream
       .on("response", (response) => {
         // Can be undefined for unknown length URLs, e.g. transfer-encoding: chunked
diff --git a/packages/@uppy/companion/lib/server/provider/google/drive/index.d.ts b/packages/@uppy/companion/lib/server/provider/google/drive/index.d.ts
index a13c57b..8ac298f 100644
--- a/packages/@uppy/companion/lib/server/provider/google/drive/index.d.ts
+++ b/packages/@uppy/companion/lib/server/provider/google/drive/index.d.ts
@@ -1,10 +1,9 @@
-export = Drive;
 /**
  * Adapter for API https://developers.google.com/drive/api/v3/
  */
-declare class Drive extends Provider {
+export class Drive extends Provider {
     list(options: any): Promise<any>;
-    download({ id: idIn, token }: {
+    download({ id, token }: {
         id: any;
         token: any;
     }): Promise<any>;
@@ -15,6 +14,16 @@ declare class Drive extends Provider {
     logout: typeof logout;
     refreshToken: typeof refreshToken;
 }
+export function streamGoogleFile({ token, id: idIn }: {
+    token: any;
+    id: any;
+}): Promise<{
+    stream: import("got", { with: { "resolution-mode": "import" } }).Request;
+}>;
+export function getGoogleFileSize({ id, token }: {
+    id: any;
+    token: any;
+}): Promise<number>;
 import Provider = require("../../Provider");
 import { logout } from "../index";
 import { refreshToken } from "../index";
diff --git a/packages/@uppy/companion/lib/server/provider/google/drive/index.js b/packages/@uppy/companion/lib/server/provider/google/drive/index.js
index e5bcbf2..d48a5cd 100644
--- a/packages/@uppy/companion/lib/server/provider/google/drive/index.js
+++ b/packages/@uppy/companion/lib/server/provider/google/drive/index.js
@@ -43,6 +43,49 @@ async function getStats({ id, token }) {
   }
   return stats;
 }
+async function streamGoogleFile({ token, id: idIn }) {
+  const client = await getClient({ token });
+  const { mimeType, id, exportLinks } = await getStats({ id: idIn, token });
+  let stream;
+  if (isGsuiteFile(mimeType)) {
+    const mimeType2 = getGsuiteExportType(mimeType);
+    logger.info(`calling google file export for ${id} to ${mimeType2}`, "provider.drive.export");
+    // GSuite files exported with large converted size results in error using standard export method.
+    // Error message: "This file is too large to be exported.".
+    // Issue logged in Google APIs: https://github.com/googleapis/google-api-nodejs-client/issues/3446
+    // Implemented based on the answer from StackOverflow: https://stackoverflow.com/a/59168288
+    const mimeTypeExportLink = exportLinks?.[mimeType2];
+    if (mimeTypeExportLink) {
+      const gSuiteFilesClient = (await got).extend({
+        headers: {
+          authorization: `Bearer ${token}`,
+        },
+      });
+      stream = gSuiteFilesClient.stream.get(mimeTypeExportLink, { responseType: "json" });
+    } else {
+      stream = client.stream.get(`files/${encodeURIComponent(id)}/export`, {
+        searchParams: { supportsAllDrives: true, mimeType: mimeType2 },
+        responseType: "json",
+      });
+    }
+  } else {
+    stream = client.stream.get(`files/${encodeURIComponent(id)}`, {
+      searchParams: { alt: "media", supportsAllDrives: true },
+      responseType: "json",
+    });
+  }
+  await prepareStream(stream);
+  return { stream };
+}
+async function getGoogleFileSize({ id, token }) {
+  const { mimeType, size } = await getStats({ id, token });
+  if (isGsuiteFile(mimeType)) {
+    // GSuite file sizes cannot be predetermined (but are max 10MB)
+    // e.g. Transfer-Encoding: chunked
+    return undefined;
+  }
+  return parseInt(size, 10);
+}
 /**
  * Adapter for API https://developers.google.com/drive/api/v3/
  */
@@ -115,7 +158,7 @@ class Drive extends Provider {
     });
   }
   // eslint-disable-next-line class-methods-use-this
-  async download({ id: idIn, token }) {
+  async download({ id, token }) {
     if (mockAccessTokenExpiredError != null) {
       logger.warn(`Access token: ${token}`);
       if (mockAccessTokenExpiredError === token) {
@@ -124,53 +167,22 @@ class Drive extends Provider {
       }
     }
     return withGoogleErrorHandling(Drive.oauthProvider, "provider.drive.download.error", async () => {
-      const client = await getClient({ token });
-      const { mimeType, id, exportLinks } = await getStats({ id: idIn, token });
-      let stream;
-      if (isGsuiteFile(mimeType)) {
-        const mimeType2 = getGsuiteExportType(mimeType);
-        logger.info(`calling google file export for ${id} to ${mimeType2}`, "provider.drive.export");
-        // GSuite files exported with large converted size results in error using standard export method.
-        // Error message: "This file is too large to be exported.".
-        // Issue logged in Google APIs: https://github.com/googleapis/google-api-nodejs-client/issues/3446
-        // Implemented based on the answer from StackOverflow: https://stackoverflow.com/a/59168288
-        const mimeTypeExportLink = exportLinks?.[mimeType2];
-        if (mimeTypeExportLink) {
-          const gSuiteFilesClient = (await got).extend({
-            headers: {
-              authorization: `Bearer ${token}`,
-            },
-          });
-          stream = gSuiteFilesClient.stream.get(mimeTypeExportLink, { responseType: "json" });
-        } else {
-          stream = client.stream.get(`files/${encodeURIComponent(id)}/export`, {
-            searchParams: { supportsAllDrives: true, mimeType: mimeType2 },
-            responseType: "json",
-          });
-        }
-      } else {
-        stream = client.stream.get(`files/${encodeURIComponent(id)}`, {
-          searchParams: { alt: "media", supportsAllDrives: true },
-          responseType: "json",
-        });
-      }
-      await prepareStream(stream);
-      return { stream };
+      return streamGoogleFile({ token, id });
     });
   }
   // eslint-disable-next-line class-methods-use-this
   async size({ id, token }) {
-    return withGoogleErrorHandling(Drive.oauthProvider, "provider.drive.size.error", async () => {
-      const { mimeType, size } = await getStats({ id, token });
-      if (isGsuiteFile(mimeType)) {
-        // GSuite file sizes cannot be predetermined (but are max 10MB)
-        // e.g. Transfer-Encoding: chunked
-        return undefined;
-      }
-      return parseInt(size, 10);
-    });
+    return withGoogleErrorHandling(
+      Drive.oauthProvider,
+      "provider.drive.size.error",
+      async () => (getGoogleFileSize({ id, token })),
+    );
   }
 }
 Drive.prototype.logout = logout;
 Drive.prototype.refreshToken = refreshToken;
-module.exports = Drive;
+module.exports = {
+  Drive,
+  streamGoogleFile,
+  getGoogleFileSize,
+};
diff --git a/packages/@uppy/companion/lib/server/provider/index.js b/packages/@uppy/companion/lib/server/provider/index.js
index 7691d83..12e8acb 100644
--- a/packages/@uppy/companion/lib/server/provider/index.js
+++ b/packages/@uppy/companion/lib/server/provider/index.js
@@ -5,7 +5,7 @@ Object.defineProperty(exports, "__esModule", { value: true });
  */
 const dropbox = require("./dropbox");
 const box = require("./box");
-const drive = require("./google/drive");
+const { Drive } = require("./google/drive");
 const googlephotos = require("./google/googlephotos");
 const instagram = require("./instagram/graph");
 const facebook = require("./facebook");
@@ -65,7 +65,7 @@ module.exports.getProviderMiddleware = (providers, grantConfig) => {
  * @returns {Record<string, typeof Provider>}
  */
 module.exports.getDefaultProviders = () => {
-  const providers = { dropbox, box, drive, googlephotos, facebook, onedrive, zoom, instagram, unsplash };
+  const providers = { dropbox, box, drive: Drive, googlephotos, facebook, onedrive, zoom, instagram, unsplash };
   return providers;
 };
 /**
diff --git a/packages/@uppy/companion/lib/standalone/helper.js b/packages/@uppy/companion/lib/standalone/helper.js
index edaa622..ccc5e1a 100644
--- a/packages/@uppy/companion/lib/standalone/helper.js
+++ b/packages/@uppy/companion/lib/standalone/helper.js
@@ -143,6 +143,7 @@ const getConfigFromEnv = () => {
       validHosts,
     },
     enableUrlEndpoint: process.env.COMPANION_ENABLE_URL_ENDPOINT === "true",
+    enableGooglePickerEndpoint: process.env.COMPANION_ENABLE_GOOGLE_PICKER_ENDPOINT === "true",
     periodicPingUrls: process.env.COMPANION_PERIODIC_PING_URLS
       ? process.env.COMPANION_PERIODIC_PING_URLS.split(",")
       : [],
diff --git a/packages/@uppy/core/lib/Uppy.js b/packages/@uppy/core/lib/Uppy.js
index abb82f2..ade1397 100644
--- a/packages/@uppy/core/lib/Uppy.js
+++ b/packages/@uppy/core/lib/Uppy.js
@@ -386,7 +386,7 @@ export class Uppy {
       ...this.getState().files,
     };
     if (!updatedFiles[fileID]) {
-      this.log("Was trying to set metadata for a file that has been removed: ", fileID);
+      this.log(`Was trying to set metadata for a file that has been removed: ${fileID}`);
       return;
     }
     const newMeta = {
diff --git a/packages/@uppy/core/lib/locale.js b/packages/@uppy/core/lib/locale.js
index 7311693..6ac4428 100644
--- a/packages/@uppy/core/lib/locale.js
+++ b/packages/@uppy/core/lib/locale.js
@@ -36,6 +36,9 @@ export default {
     openFolderNamed: "Open folder %{name}",
     cancel: "Cancel",
     logOut: "Log out",
+    logIn: "Log in",
+    pickFiles: "Pick files",
+    pickPhotos: "Pick photos",
     filter: "Filter",
     resetFilter: "Reset filter",
     loading: "Loading...",
@@ -56,5 +59,6 @@ export default {
     },
     additionalRestrictionsFailed: "%{count} additional restrictions were not fulfilled",
     unnamed: "Unnamed",
+    pleaseWait: "Please wait",
   },
 };
diff --git a/packages/@uppy/provider-views/lib/index.js b/packages/@uppy/provider-views/lib/index.js
index 3e5b730..9d7cc3d 100644
--- a/packages/@uppy/provider-views/lib/index.js
+++ b/packages/@uppy/provider-views/lib/index.js
@@ -1,2 +1,3 @@
+export { default as GooglePickerView } from "./GooglePicker/GooglePickerView.js";
 export { default as ProviderViews, defaultPickerIcon } from "./ProviderView/index.js";
 export { default as SearchProviderViews } from "./SearchProviderView/index.js";
diff --git a/packages/@uppy/transloadit/lib/index.js b/packages/@uppy/transloadit/lib/index.js
index 2e8d13d..3a923d3 100644
--- a/packages/@uppy/transloadit/lib/index.js
+++ b/packages/@uppy/transloadit/lib/index.js
@@ -459,6 +459,8 @@ function _getClientVersion2() {
   addPluginVersion("Facebook", "uppy-facebook");
   addPluginVersion("GoogleDrive", "uppy-google-drive");
   addPluginVersion("GooglePhotos", "uppy-google-photos");
+  addPluginVersion("GoogleDrivePicker", "uppy-google-drive-picker");
+  addPluginVersion("GooglePhotosPicker", "uppy-google-photos-picker");
   addPluginVersion("Instagram", "uppy-instagram");
   addPluginVersion("OneDrive", "uppy-onedrive");
   addPluginVersion("Zoom", "uppy-zoom");

- split into two plugins
- implement photos picker
- auto login
- save access token in local storage
- document
- handle photos/files picked and send to companion
- add new hook useStore for making it easier to use localStorage data in react
- add new hook useUppyState for making it easier to use uppy state from react
- add new hook useUppyPluginState for making it easier to plugin state from react
- fix css error
which occurs in dev when js has been built before build:ts gets called
@mifi mifi changed the title Draft: Google Picker Google Picker Nov 18, 2024
@mifi
Copy link
Contributor Author

mifi commented Nov 18, 2024

I think this is ready for review. The remaining points can be done iteratively in future PRs

@mifi mifi requested a review from Murderlon November 18, 2024 12:50
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lot of hard work to get this, well done!

Initial review below, still need to test it locally. Two Qs:

  1. Can you add the google account setup to our [email protected] account? Everyone already has those credentials setup so that would be great.
  2. If you use both google drive and google photos, do we get duplicate requests for the scripts you're loading?


import type { AsyncStore } from './Uppy'

export default function useStore(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this in core? And If you want to sync a store into preact/react state that's what useSyncExternalStore is for?

import type { State, Uppy } from './Uppy'

// todo merge with @uppy/react?
export function useUppyState<
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be in core either. And we should try to avoid writing the exact same hooks from @uppy/react again.

}, [accessToken, clientId, pickerType, setAccessToken, uppy])

useEffect(() => {
;(async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should try to keep as much async work out of the React render cycle as possible. It's tricky because of cancellation during re-renders, loading, error, and retrying that has to be manually synced with state and you just keep fighting useEffect, which become fragile if they start to depend on too many things.

This comment is applicable to a lot of code in this file.

It would be great if we can move as much as possible into the plugin class and keep the Preact UI simple, ideally also preventing us from duplicating new hooks.

Copy link

@StrixOSG StrixOSG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to add on to @Murderlon's review. I was testing locally here and I seem to get a 403 after maybe about an hour or so for Google Drive Picker. I got everything working properly except that once the access token expires after about that hour I have to go into my local storage and clear uppy:google-drive-picker:accessToken manually otherwise it doesn't allow me to even logout and sign back in. I've attached a screenshot here of the error but it looks like it doesn't even hit the API, it's just a 403 for Google Picker account selection according to the Network request. So it might need an expiry date attached so it can be checked on the client side? Otherwise I don't know how this could be refreshed.

Screenshot 2024-11-21 at 1 20 00 PM

network tab for google picker 403

An aside and maybe it's just my UI or CSS config but the login, pick files, logout buttons do not look great as they're just plain buttons. Thank you for your hard work on this @mifi, it's much appreciated.

…icker

# Conflicts:
#	docs/companion.md
#	docs/sources/companion-plugins/box.mdx
#	docs/sources/companion-plugins/dropbox.mdx
#	docs/sources/companion-plugins/facebook.mdx
#	docs/sources/companion-plugins/google-drive.mdx
#	docs/sources/companion-plugins/google-photos.mdx
#	docs/sources/companion-plugins/instagram.mdx
#	docs/sources/companion-plugins/onedrive.mdx
#	docs/sources/companion-plugins/unsplash.mdx
#	docs/sources/companion-plugins/url.mdx
#	docs/sources/companion-plugins/zoom.mdx
#	docs/user-interfaces/dashboard.mdx
mifi added a commit to transloadit/uppy.io that referenced this pull request Nov 26, 2024
@mifi
Copy link
Contributor Author

mifi commented Nov 26, 2024

I have now:

  • implemented a check for whether the token is valid before opening google drive (as it evidently crashes hard and doesn't report that the token is expired in any way)
  • pulled out most async logic from the react component into async functions with a simpler interface. GooglePickerView is now only 200 loc. Note that we still need useEffects because:
    • I want to be nice to the user and auto-open the picker/auth dialog when the plugin is selected (ui mounted). Not sure how that can be done from inside the Plugin.
    • for google photos: as long as the UI is visible, we have to continuously poll the Google picker API to see whether the user has chosen any photos - there seems to be a shortcoming in the Picker API where there is no way of knowing when the user has closed the Picker window, so we can't really stop polling until the Uppy Google Photos UI has been closed by the user (component unmounted), or else we might miss some photo pick events. I'm not sure how to correctly control this behaviour from the Plugin.
  • removed docs from this PR (as docs have been removed from this repo)
  • implemented a better UI / buttons
  • removed unused useUppyState hook
  • I'm not sure where to put the useStore hook, but because I see that @uppy/core already depends on preact, I thought it's ok. I could put it in utils, but I'm not sure what's the benefit of that. Putting it in @provider-views feels wrong because it's a hook that can be used by all preact code (not just providers).

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.

Google Photos API Deprecation Mar. 31, 2025 Implement Google Picker API
3 participants