kth / canvas-api Goto Github PK

I originally opened an issue about this in my org's project repo (see tl-its-umich-edu/canvas-course-manager-next#159) because I thought we were not using KTH/canvas-api correctly. However, a colleague pointed out to me that your code is responsible for the response handling, so I'm opening this new issue.

When a Canvas API function is called without pagination handling via get(), the got Response object that was returned had a statusCode. It seems that this list() method doesn't return that type of object, so that property (and others that may be useful) isn't available.

Also, it seems to me that list() should return a promise for a got Response object (or array of them) and the toArray() method could be called later to make an array of data from those responses.

For example, I'd expect to use it something like this…

      const response = await requestor.list<CanvasCourseSection>(endpoint, queryParams)
      logger.debug(`Received response with status code ${response.statusCode}`)
      const sections = response.toArray().map(s => ({
      …

(Apologies in advance for my typos. TypeScript syntax is new to me.)

class CanvasApiError extends Error {
    constructor(gotError) {
        super(gotError.message);
        this.name = "CanvasApiError";
        this.code = gotError.code; //  <--- suggestion
        this.options = {
            headers: gotError.options.headers,
            url: gotError.options.url.toString(),
        };
        this.response = gotError.response;
        this.options.headers.authorization = "[HIDDEN]";
    }
}

Right now we don't get much discrimination between types of errors. Using the message feels brittle.

Hi @kth/canvas-api team! Let me preface this issue by saying my team at the University of Michigan has been using your library, and has thus far found it quite useful. I hope you see me filing this issue more as a contribution than a complaint.

I've been working on some dependency updates, and I saw in a few of your pre-releases that you are making the switch to TypeScript, which is great for us since the project we're utilizing this library in is all TypeScript. However, when trying to adopt 3.3.0-1, I encountered an issue where I couldn't use error instanceof CanvasApiError (it returned an error saying "Right-hand side of 'instanceof' is not an object"), which is important to our use case. Note that I did not have this problem with the 4.0.0-0 pre-release, so I looked at the difference to see what might have changed: v4.0.0-0...v3.3.0-1 . I believe the module.exports = CanvasApi at the bottom may be the culprit, but I am not an expert on exports/modules in JS/TS.

I saw that your test is passing when CanvasApiError is imported from ./utils (it also works for me in our project if we import from @kth/canvas-api/dist/utils), but I think the intention was for this to be available from the root (since it's re-exported in index.ts). If you modify your test to import from ./index.ts, you can reproduce the problem.

+++ b/src/errorHandler.test.ts
@@ -1,6 +1,5 @@
 import createTestServer from "create-test-server";
-import Canvas from "./index";
-import { CanvasApiError } from "./utils";
+import Canvas, { CanvasApiError } from "./index";

 test("errorHandler converts HTTPError to CanvasApiError", async () => {
   const server = await createTestServer();
@@ -13,6 +12,8 @@ test("errorHandler converts HTTPError to CanvasApiError", async () => {
   try {
     await canvas.get("something");
   } catch (err) {
+    console.log(err.constructor.name);
+    console.log(typeof CanvasApiError);
     expect(err).toBeInstanceOf(CanvasApiError);
     expect(err).toMatchInlineSnapshot(
       `[CanvasApiError: Response code 401 (Unauthorized)]`
(END)

If you're open to contributions, I can open a PR against ts-compat with this commit: ssciolla@7f5c0e7

Anyways, hope that is helpful. Do you have plans to release 4.0.0 (not a pre-release) in the near future? Thanks for sharing this project!

Missing /dist folder in package published on Npmjs.