Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

throttling MosaicClient.requestQuery errors when requestQuery does not return a Promise #507

Closed
frtennis1 opened this issue Aug 29, 2024 · 2 comments

Comments

@frtennis1
Copy link
Contributor

The symptom I'm seeing (sorry, I don't have a good MRE for this) is a,

TypeError: callback(...).then is not a function

raised from throttle when the callback is this.requestQuery(). I think this is because throttle expects a callback that returns a promise, but MosaicClient.requestQuery() does not always return a callback. In particular,

  • MosaicClient.requestQuery calls Coordinator.requestQuery
  • when query is empty, Coordiantor.requestQuery returns client.update()
  • client.update() sometimes returns this instead of a Promise (for the base class and Menu at least)

I don't know that this is the best way to address it, but for what it's worth, re-running with the following patch no longer triggers this error,

diff --git a/packages/core/src/MosaicClient.js b/packages/core/src/MosaicClient.js
index 8b71e09..8dd8c03 100644
--- a/packages/core/src/MosaicClient.js
+++ b/packages/core/src/MosaicClient.js
@@ -135,6 +135,6 @@ export class MosaicClient {
    * @returns {this | Promise<any>}
    */
   update() {
-    return this;
+    return Promise.resolve(this);
   }
 }
diff --git a/packages/inputs/src/Menu.js b/packages/inputs/src/Menu.js
index 5ed3fea..8054250 100644
--- a/packages/inputs/src/Menu.js
+++ b/packages/inputs/src/Menu.js
@@ -162,10 +162,10 @@ export class Menu extends MosaicClient {
     if (selection) {
       const value = isSelection(selection)
         ? selection.valueFor(this)
-        : selection.value;
+        : selection.value
       this.selectedValue(value ?? '');
     }

-    return this;
+    return Promise.resolve(this);
   }
 }
@domoritz
Copy link
Member

Just as a side note, you can call await a promise or a value and both work. So maybe we can replace then with await.

@jheer jheer closed this as completed in e91a862 Sep 13, 2024
@jheer
Copy link
Member

jheer commented Sep 13, 2024

Thanks! I updated Coordinator.requestQuery to always return a Promise -- here a resolved Promise with the value of client.update(), as this parallels what updateClient returns.

@jheer jheer mentioned this issue Sep 16, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants