PartialTree - get rid of `.onFirstRender()` #5187

lakesare · 2024-05-21T17:47:37Z

This is a series of PRs that make the #5050 PR smaller

This PR gets rid of .onFirstRender() method.

Rationale

The code for .onFirstRender() repeats in all provders, the only thing changing is provider.rootFolderId
In the Provider views rewrite (.files, .folders => .partialTree) #5050 PR, we will need provider.rootFolderId as a separate entity
Having .onFirstRender() makes it so that:
- Dropbox.tsx depends on code in ProviderView.tsx (.getFolder()) AND
- ProviderView.tsx depends on code in Dropbox.tsx (.onFirstRender())
It's easier to reason about code when the dependency is one-directional.

Notes

This is a breaking change, but only for people who implement their own providers.

Checks

I checked it locally with all providers:

github-actions · 2024-05-21T17:48:57Z

Diff output files

diff --git a/packages/@uppy/box/lib/Box.js b/packages/@uppy/box/lib/Box.js
index d074a02..0a0cf13 100644
--- a/packages/@uppy/box/lib/Box.js
+++ b/packages/@uppy/box/lib/Box.js
@@ -9,6 +9,7 @@ const packageJson = {
 export default class Box extends UIPlugin {
   constructor(uppy, opts) {
     super(uppy, opts);
+    this.rootFolderId = null;
     this.id = this.opts.id || "Box";
     this.type = "acquirer";
     this.storage = this.opts.storage || tokenStorage;
@@ -51,7 +52,6 @@ export default class Box extends UIPlugin {
     this.defaultLocale = locale;
     this.i18nInit();
     this.title = this.i18n("pluginNameBox");
-    this.onFirstRender = this.onFirstRender.bind(this);
     this.render = this.render.bind(this);
   }
   install() {
@@ -70,9 +70,6 @@ export default class Box extends UIPlugin {
     this.view.tearDown();
     this.unmount();
   }
-  async onFirstRender() {
-    await Promise.all([this.provider.fetchPreAuthToken(), this.view.getFolder()]);
-  }
   render(state) {
     return this.view.render(state);
   }
diff --git a/packages/@uppy/dropbox/lib/Dropbox.js b/packages/@uppy/dropbox/lib/Dropbox.js
index 116970e..82ec6a1 100644
--- a/packages/@uppy/dropbox/lib/Dropbox.js
+++ b/packages/@uppy/dropbox/lib/Dropbox.js
@@ -9,6 +9,7 @@ const packageJson = {
 export default class Dropbox extends UIPlugin {
   constructor(uppy, opts) {
     super(uppy, opts);
+    this.rootFolderId = null;
     this.id = this.opts.id || "Dropbox";
     this.type = "acquirer";
     this.storage = this.opts.storage || tokenStorage;
@@ -43,7 +44,6 @@ export default class Dropbox extends UIPlugin {
     this.defaultLocale = locale;
     this.i18nInit();
     this.title = this.i18n("pluginNameDropbox");
-    this.onFirstRender = this.onFirstRender.bind(this);
     this.render = this.render.bind(this);
   }
   install() {
@@ -62,9 +62,6 @@ export default class Dropbox extends UIPlugin {
     this.view.tearDown();
     this.unmount();
   }
-  async onFirstRender() {
-    await Promise.all([this.provider.fetchPreAuthToken(), this.view.getFolder()]);
-  }
   render(state) {
     return this.view.render(state);
   }
diff --git a/packages/@uppy/facebook/lib/Facebook.js b/packages/@uppy/facebook/lib/Facebook.js
index b02a8c9..4515abf 100644
--- a/packages/@uppy/facebook/lib/Facebook.js
+++ b/packages/@uppy/facebook/lib/Facebook.js
@@ -9,6 +9,7 @@ const packageJson = {
 export default class Facebook extends UIPlugin {
   constructor(uppy, opts) {
     super(uppy, opts);
+    this.rootFolderId = null;
     this.id = this.opts.id || "Facebook";
     this.type = "acquirer";
     this.storage = this.opts.storage || tokenStorage;
@@ -52,7 +53,6 @@ export default class Facebook extends UIPlugin {
     this.defaultLocale = locale;
     this.i18nInit();
     this.title = this.i18n("pluginNameFacebook");
-    this.onFirstRender = this.onFirstRender.bind(this);
     this.render = this.render.bind(this);
   }
   install() {
@@ -70,9 +70,6 @@ export default class Facebook extends UIPlugin {
     this.view.tearDown();
     this.unmount();
   }
-  async onFirstRender() {
-    await Promise.all([this.provider.fetchPreAuthToken(), this.view.getFolder()]);
-  }
   render(state) {
     const viewOptions = {};
     if (this.getPluginState().files.length && !this.getPluginState().folders.length) {
diff --git a/packages/@uppy/google-drive/lib/GoogleDrive.js b/packages/@uppy/google-drive/lib/GoogleDrive.js
index a77c36e..c8ad8f3 100644
--- a/packages/@uppy/google-drive/lib/GoogleDrive.js
+++ b/packages/@uppy/google-drive/lib/GoogleDrive.js
@@ -9,6 +9,7 @@ const packageJson = {
 export default class GoogleDrive extends UIPlugin {
   constructor(uppy, opts) {
     super(uppy, opts);
+    this.rootFolderId = "root";
     this.type = "acquirer";
     this.storage = this.opts.storage || tokenStorage;
     this.files = [];
@@ -68,7 +69,6 @@ export default class GoogleDrive extends UIPlugin {
     this.defaultLocale = locale;
     this.i18nInit();
     this.title = this.i18n("pluginNameGoogleDrive");
-    this.onFirstRender = this.onFirstRender.bind(this);
     this.render = this.render.bind(this);
   }
   install() {
@@ -87,9 +87,6 @@ export default class GoogleDrive extends UIPlugin {
     this.view.tearDown();
     this.unmount();
   }
-  async onFirstRender() {
-    await Promise.all([this.provider.fetchPreAuthToken(), this.view.getFolder("root")]);
-  }
   render(state) {
     return this.view.render(state);
   }
diff --git a/packages/@uppy/instagram/lib/Instagram.js b/packages/@uppy/instagram/lib/Instagram.js
index 30f6be9..57e1667 100644
--- a/packages/@uppy/instagram/lib/Instagram.js
+++ b/packages/@uppy/instagram/lib/Instagram.js
@@ -9,6 +9,7 @@ const packageJson = {
 export default class Instagram extends UIPlugin {
   constructor(uppy, opts) {
     super(uppy, opts);
+    this.rootFolderId = "recent";
     this.type = "acquirer";
     this.files = [];
     this.storage = this.opts.storage || tokenStorage;
@@ -71,7 +72,6 @@ export default class Instagram extends UIPlugin {
       pluginId: this.id,
       supportsRefreshToken: false,
     });
-    this.onFirstRender = this.onFirstRender.bind(this);
     this.render = this.render.bind(this);
   }
   install() {
@@ -93,9 +93,6 @@ export default class Instagram extends UIPlugin {
     this.view.tearDown();
     this.unmount();
   }
-  async onFirstRender() {
-    await Promise.all([this.provider.fetchPreAuthToken(), this.view.getFolder("recent")]);
-  }
   render(state) {
     return this.view.render(state);
   }
diff --git a/packages/@uppy/onedrive/lib/OneDrive.js b/packages/@uppy/onedrive/lib/OneDrive.js
index 5fee250..1aedc07 100644
--- a/packages/@uppy/onedrive/lib/OneDrive.js
+++ b/packages/@uppy/onedrive/lib/OneDrive.js
@@ -9,6 +9,7 @@ const packageJson = {
 export default class OneDrive extends UIPlugin {
   constructor(uppy, opts) {
     super(uppy, opts);
+    this.rootFolderId = null;
     this.type = "acquirer";
     this.files = [];
     this.storage = this.opts.storage || tokenStorage;
@@ -60,7 +61,6 @@ export default class OneDrive extends UIPlugin {
     this.defaultLocale = locale;
     this.i18nInit();
     this.title = this.i18n("pluginNameOneDrive");
-    this.onFirstRender = this.onFirstRender.bind(this);
     this.render = this.render.bind(this);
   }
   install() {
@@ -79,9 +79,6 @@ export default class OneDrive extends UIPlugin {
     this.view.tearDown();
     this.unmount();
   }
-  async onFirstRender() {
-    await Promise.all([this.provider.fetchPreAuthToken(), this.view.getFolder()]);
-  }
   render(state) {
     return this.view.render(state);
   }
diff --git a/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js b/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js
index b3b36f9..670029d 100644
--- a/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js
+++ b/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js
@@ -206,7 +206,7 @@ export default class ProviderView extends View {
         this.plugin.setPluginState({
           authenticated: true,
         });
-        this.preFirstRender();
+        await this.getFolder(this.plugin.rootFolderId || undefined);
       });
     } catch (err) {
       if (err.name === "UserFacingApiError") {
@@ -346,7 +346,11 @@ export default class ProviderView extends View {
       i18n,
     } = this.plugin.uppy;
     if (!didFirstRender) {
-      this.preFirstRender();
+      this.plugin.setPluginState({
+        didFirstRender: true,
+      });
+      this.provider.fetchPreAuthToken();
+      this.getFolder(this.plugin.rootFolderId || undefined);
     }
     const targetViewOptions = {
       ...this.opts,
diff --git a/packages/@uppy/provider-views/lib/SearchProviderView/SearchProviderView.js b/packages/@uppy/provider-views/lib/SearchProviderView/SearchProviderView.js
index b328100..c27fa47 100644
--- a/packages/@uppy/provider-views/lib/SearchProviderView/SearchProviderView.js
+++ b/packages/@uppy/provider-views/lib/SearchProviderView/SearchProviderView.js
@@ -111,16 +111,12 @@ export default class SearchProviderView extends View {
       viewOptions = {};
     }
     const {
-      didFirstRender,
       isInputMode,
       searchTerm,
     } = this.plugin.getPluginState();
     const {
       i18n,
     } = this.plugin.uppy;
-    if (!didFirstRender) {
-      this.preFirstRender();
-    }
     const targetViewOptions = {
       ...this.opts,
       ...viewOptions,
diff --git a/packages/@uppy/provider-views/lib/View.js b/packages/@uppy/provider-views/lib/View.js
index 69cd639..f3395cb 100644
--- a/packages/@uppy/provider-views/lib/View.js
+++ b/packages/@uppy/provider-views/lib/View.js
@@ -25,17 +25,10 @@ export default class View {
     this.provider = opts.provider;
     this.opts = opts;
     this.isHandlingScroll = false;
-    this.preFirstRender = this.preFirstRender.bind(this);
     this.handleError = this.handleError.bind(this);
     this.clearSelection = this.clearSelection.bind(this);
     this.cancelPicking = this.cancelPicking.bind(this);
   }
-  preFirstRender() {
-    this.plugin.setPluginState({
-      didFirstRender: true,
-    });
-    this.plugin.onFirstRender();
-  }
   shouldHandleScroll(event) {
     const {
       scrollHeight,
diff --git a/packages/@uppy/unsplash/lib/Unsplash.js b/packages/@uppy/unsplash/lib/Unsplash.js
index 0aeab85..81930c9 100644
--- a/packages/@uppy/unsplash/lib/Unsplash.js
+++ b/packages/@uppy/unsplash/lib/Unsplash.js
@@ -65,7 +65,6 @@ export default class Unsplash extends UIPlugin {
       this.mount(target, this);
     }
   }
-  async onFirstRender() {}
   render(state) {
     return this.view.render(state);
   }
diff --git a/packages/@uppy/zoom/lib/Zoom.js b/packages/@uppy/zoom/lib/Zoom.js
index 7a33ad4..c65356b 100644
--- a/packages/@uppy/zoom/lib/Zoom.js
+++ b/packages/@uppy/zoom/lib/Zoom.js
@@ -9,6 +9,7 @@ const packageJson = {
 export default class Zoom extends UIPlugin {
   constructor(uppy, opts) {
     super(uppy, opts);
+    this.rootFolderId = null;
     this.type = "acquirer";
     this.files = [];
     this.storage = this.opts.storage || tokenStorage;
@@ -42,7 +43,6 @@ export default class Zoom extends UIPlugin {
     this.defaultLocale = locale;
     this.i18nInit();
     this.title = this.i18n("pluginNameZoom");
-    this.onFirstRender = this.onFirstRender.bind(this);
     this.render = this.render.bind(this);
   }
   install() {
@@ -60,9 +60,6 @@ export default class Zoom extends UIPlugin {
     this.view.tearDown();
     this.unmount();
   }
-  async onFirstRender() {
-    await Promise.all([this.provider.fetchPreAuthToken(), this.view.getFolder()]);
-  }
   render(state) {
     return this.view.render(state);
   }

Murderlon · 2024-05-23T13:31:07Z

Is this PR accidentally still on draft, or should we hold off with a review until more changes?

lakesare · 2024-05-23T20:05:17Z

@Murderlon, I don't draft PRs on accident, I'll invite reviewers once it's ready :-)
Preparing a new .env so that it's easier on reviewers.

Murderlon

Code looks good but because this is a breaking change, we'd have to land it in 4.x branch. Which may or may not mean all your changes should land there.

lakesare · 2024-05-30T00:53:37Z

Agreed, moved this PR to 4.x.

#5050 has some UI changes that can be considered a breaking change, maybe putting the bulk of #5050 into 4.x is a good idea.

| Package | Version | Package | Version | | ---------------------- | ------------- | ---------------------- | ------------- | | @uppy/angular | 0.7.0-beta.5 | @uppy/instagram | 4.0.0-beta.6 | | @uppy/audio | 2.0.0-beta.6 | @uppy/locales | 4.0.0-beta.2 | | @uppy/aws-s3 | 4.0.0-beta.5 | @uppy/onedrive | 4.0.0-beta.6 | | @uppy/aws-s3-multipart | 4.0.0-beta.6 | @uppy/provider-views | 4.0.0-beta.7 | | @uppy/box | 3.0.0-beta.6 | @uppy/status-bar | 4.0.0-beta.9 | | @uppy/companion | 5.0.0-beta.9 | @uppy/transloadit | 4.0.0-beta.7 | | @uppy/companion-client | 4.0.0-beta.7 | @uppy/tus | 4.0.0-beta.6 | | @uppy/core | 4.0.0-beta.9 | @uppy/unsplash | 4.0.0-beta.7 | | @uppy/dashboard | 4.0.0-beta.9 | @uppy/url | 4.0.0-beta.7 | | @uppy/drop-target | 3.0.0-beta.5 | @uppy/utils | 6.0.0-beta.8 | | @uppy/dropbox | 4.0.0-beta.7 | @uppy/webcam | 4.0.0-beta.8 | | @uppy/facebook | 4.0.0-beta.6 | @uppy/xhr-upload | 4.0.0-beta.6 | | @uppy/form | 4.0.0-beta.4 | @uppy/zoom | 3.0.0-beta.6 | | @uppy/golden-retriever | 4.0.0-beta.5 | uppy | 4.0.0-beta.10 | | @uppy/google-drive | 4.0.0-beta.6 | | | - @uppy/audio: remove unused component props (Antoine du Hamel / #5209) - @uppy/angular: fix invalid char in `package.json` (Antoine du Hamel / #5224) - meta: use default argument value instead of `defaultProps` (Antoine du Hamel / #5222) - @uppy/angular: upgrade to Angular 18 (Antoine du Hamel / #5215) - @uppy/utils: remove unused `settle` (Antoine du Hamel / #5210) - @uppy/form: move internal property to private field (Antoine du Hamel / #5214) - @uppy/dashboard: remove unused component props (Antoine du Hamel / #5213) - @uppy/status-bar: remove unused component props (Antoine du Hamel / #5211) - @uppy/audio: move internal property to private field (Antoine du Hamel / #5207) - @uppy/aws-s3: remove todo (Mikael Finstad / #5200) - @uppy/core: remove unnecessary todo (Mikael Finstad / #5200) - @uppy/aws-s3: do not expose internal `assertHost` method (Mikael Finstad / #5200) - @uppy/aws-s3: make passing `signal` consistent (Mikael Finstad / #5200) - @uppy/core: remove `'upload-started'` event (Mikael Finstad / #5200) - @uppy/aws-s3: remove `chunkState` getter (Mikael Finstad / #5200) - @uppy/drop-target: remove `title` property (Mikael Finstad / #5200) - @uppy/golden-retriever: remove unused `ready` setters (Mikael Finstad / #5200) - @uppy/dashboard: remove deprecated `autoOpenFileEditor` option (Mikael Finstad / #5200) - @uppy/aws-s3: remove `uploaderSockets` (Mikael Finstad / #5200) - @uppy/locales: remove hacks for legacy bundle (Mikael Finstad / #5200) - @uppy/status-bar: rename `StatusBar` to `StatusBarUI` (Mikael Finstad / #5200) - @uppy/url: remove unused error handler (Mikael Finstad / #5200) - @uppy/aws-s3,@uppy/tus,@uppy/utils,@uppy/xhr-upload: remove `uploader` from `upload-progress` event (Mikael Finstad / #5200) - @uppy/webcam: remove `facingMode` option (Mikael Finstad / #5200) - @uppy/companion: invert some internal boolean options (Mikael Finstad / #5198) - @uppy/companion: rename `authProvider` to `oauthProvider` (Mikael Finstad / #5198) - @uppy/companion: remove unused headers (Mikael Finstad / #5198) - @uppy/companion: remove sanitizing of metadata (Mikael Finstad / #5198) - @uppy/companion-client: do not allow boolean `RequestOptions` (Mikael Finstad / #5198) - @uppy/companion-client: remove deprecated options (Mikael Finstad / #5198) - @uppy/companion: remove `error.extraData` (Mikael Finstad / #5198) - @uppy/companion-client: make `supportsRefreshToken` default (Mikael Finstad / #5198) - @uppy/companion-client: remove optional chaining (Mikael Finstad / #5198) - @uppy/companion: capitalize POST (Mikael Finstad / #5198) - @uppy/companion: simplify code by using modern Node.js APIs (Mikael Finstad / #5198) - @uppy/companion-client: remove `Socket` (Mikael Finstad / #5198) - @uppy/companion: rename `getExtraConfig` to `getExtraGrantConfig` (Mikael Finstad / #5198) - @uppy/companion: change `COMPANION_ENABLE_URL_ENDPOINT` default (Mikael Finstad / #5198) - @uppy/companion: change default value for Redis session prefix (Mikael Finstad / #5198) - examples: make React example up-to-date (Merlijn Vos / #5205) - @uppy/core: add type tests (Merlijn Vos / #5153) - @uppy/provider-views: PartialTree - get rid of `.onFirstRender()` (Evgenia Karunus / #5187) - @uppy/core: pass file to events consistently (Merlijn Vos / #5136) - docs: assume tree-shaking bundler is the most common case (Antoine du Hamel / #5160) - @uppy/core: remove `reason` (Antoine du Hamel / #5159) - @uppy/core: remove `resetProgress` and `reset-progress` (Mikael Finstad / #5221)

lakesare requested a review from Murderlon May 29, 2024 01:29

lakesare marked this pull request as ready for review May 29, 2024 01:29

lakesare requested a review from mifi May 29, 2024 01:29

Murderlon reviewed May 29, 2024

View reviewed changes

lakesare changed the base branch from main to 4.x May 30, 2024 00:44

everywhere - get rid of .onFirstRender()

5351dd5

lakesare force-pushed the lakesare/partialTree-onFirstRender branch from fe08cf1 to 5351dd5 Compare May 30, 2024 01:48

lakesare merged commit b7b65b9 into 4.x May 30, 2024
17 checks passed

lakesare deleted the lakesare/partialTree-onFirstRender branch May 30, 2024 02:06

github-actions bot mentioned this pull request Jun 4, 2024

Release: uppy@4.0.0-beta.10 #5225

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PartialTree - get rid of `.onFirstRender()` #5187

PartialTree - get rid of `.onFirstRender()` #5187

lakesare commented May 21, 2024 •

edited

Loading

github-actions bot commented May 21, 2024 •

edited

Loading

Murderlon commented May 23, 2024

lakesare commented May 23, 2024

Murderlon left a comment

lakesare commented May 30, 2024 •

edited

Loading

PartialTree - get rid of .onFirstRender() #5187

PartialTree - get rid of .onFirstRender() #5187

Conversation

lakesare commented May 21, 2024 • edited Loading

Rationale

Notes

Checks

github-actions bot commented May 21, 2024 • edited Loading

Murderlon commented May 23, 2024

lakesare commented May 23, 2024

Murderlon left a comment

Choose a reason for hiding this comment

lakesare commented May 30, 2024 • edited Loading

PartialTree - get rid of `.onFirstRender()` #5187

PartialTree - get rid of `.onFirstRender()` #5187

lakesare commented May 21, 2024 •

edited

Loading

github-actions bot commented May 21, 2024 •

edited

Loading

lakesare commented May 30, 2024 •

edited

Loading