Convenient Pagination API, Browser Support, Missing API fields

[pyrogenic]

Several additions to Discojs that I've accumulated over the years building Elephant. It's a browser-based tool to manage a Discogs collection and store listings. (source)

• Add an all API to Discojs that retrieves all pages of a paginated endpoint given the first page:

  function updateCollection() {
    return client.listItemsInFolder(0, undefined, {
      perPage: 100,
    }).then(((r) => client.all("releases", r, addToCollection)), setError);
  }

• Add a model for the error format used by Discogs to allow better error reporting
• Add a few missing fields to various responses (notes to FolderReleases, name to Artist, avatar_url to User)
• Fill out the model for marketplace listings, which did not include any of the owner-only fields
• Fix nesting of the model for per-track information
• Add the option to provide a ResultCache which may be used to skip retrieving results multiple times. (My app stores GET responses using IndexedDB.)
• Tweak the package scripts to work on Windows (I develop on several platforms)

Happy to make changes to allow the PR to be accepted.

[aknorw]

@pyrogenic Sorry for missing this one, and thanks for all the improvements!
Would it be possible to split this PR into multiple ones so it'd be easier to review?
Happy to help!

[aknorw]

@pyrogenic I spent some time yesterday introducing some changes initiated in this PR:

• Add missing fields (see comments for ones I did not add)
• Improve error reporting
• Introduce result cache
• Add allowUnsafeHeaders option
• Introduce dedicated methods to iterate through pages (see comment in the review about the implemenation difference)

Let me know what you think about them!

[aknorw]

We already have a Pagination type, named this one PaginationResponse.

[aknorw]

I chose not to introduce this change yet, as this is only the case when users are authenticated. IMHO we should have 2 different codecs/types depending on whether the user is authenticated.

[pyrogenic]

Ah, I didn't know that difference, I only saw it not match. Is it the user being authenticated at all, or the authenticated user being the owner of the listing?

I'll check out the code paths I encounter this and see what might make sense.

[aknorw]

I felt this implementation was a bit hard to follow, as you first have to call a method, then call client.all.
Instead of doing that, I introduced dedicated all methods that returns async iterators:

• getAllMasterVersions
• getAllArtistReleases
• getAllLabelReleases
• getAllRecentExports
• getAllInventoryForUser
• getAllInventory
• listAllOrders
• listAllItemsByReleaseForUser
• listAllItemsByRelease
• listAllItemsInFolderForUser
• listAllItemsInFolder
• getAllSubmissionsForUser
• getAllSubmissions
• getAllContributionsForUser
• getAllContributions
• getAllListsForUser
• getAllLists
• getAllWantlistForUser
• getAllWantlist

The example you have in the PR description would become:

async function updateCollection() {
  for await (const { releases } of client.listAllItemsInFolder()) {
    for (const release of releases) {
      await addToCollection(release)
    }
  }
}

[pyrogenic]

That seems much more user-friendly, if less magical. 😉

I probably chose my method based on writing the least code and exercising some TypeScript-fu.

[aknorw]

What is the purpose of this?

[pyrogenic]

I'm guessing there was a situation in some browser where accessing Buffer to calculate content length through. I honestly don't remember the details. Getting Chrome to be happy with those headers and their capitalization was a PITA. I myself would reject this without an explanatory comment about the circumstances of the error and the expected alternate behavior...

[pyrogenic]

@aknorw thanks so much for taking the time to review, and sorry for the delayed response. I'll take a closer look outside work hours!

... and apologies for throwing a couple years of tweaks up in a single PR!