Skip to content
This repository has been archived by the owner on Aug 15, 2024. It is now read-only.

Feature/typescript #143

Merged
merged 120 commits into from
Nov 26, 2020
Merged

Feature/typescript #143

merged 120 commits into from
Nov 26, 2020

Conversation

samijaber
Copy link
Contributor

@samijaber samijaber commented Nov 13, 2020

Overview

  • 🚧

TO-DO

  • update README
  • complete breaking changes migration guide
  • check what gets exported by the dist & how (including enums such as Language and string literal unions for args)
  • tests
  • look into removing dependency on node built-in url module
  • document:
    • special response shape for feeds
    • error sources
  • allow empty object for optional params on methods

Copy link
Member

@lukechesser lukechesser left a comment

Choose a reason for hiding this comment

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

Outside of the methods we don't implement, are there any breaking changes?

LICENSE Outdated Show resolved Hide resolved
src/methods/photos/index.ts Outdated Show resolved Hide resolved
@samijaber
Copy link
Contributor Author

Outside of the methods we don't implement, are there any breaking changes?

There's more! It's pretty much all changed. I'll need to write up a new readme of course. Biggest things:

  • removed a lot of methods
  • renamed all existing methods & changed their arguments
  • changed the response type of the Promise for all the methods:
    • it now automatically calls response.json() if the response is valid JSON and returns that as its output
    • it does some basic error-handling now.
  • added the ability to abort network requests
  • the API is an object instead of a class, so creating an instance is no longer done using new Unsplash()

A very quick overview of how to consume response from the new API:

const api = Unsplash({ accessKey: 'abc123' });

api.photos.get({ photoId: 'foo' }).then(result => {
  if (result.type === 'error') {
    console.log('error occurred: ', result.errors[0]);
  } else if (result.type === 'aborted') {
    console.log('fetch request aborted');
  } else {
    console.log('received photo: ', result.response);
  }
});

One other thing I am also adding is for feed methods: the lib will automatically handle extracting the total from x-total header and return that along with the JSON body (e.g. result.response.feed and result.response.total) as that's annoying manual work that any Unsplash API consumer that shows a feed will likely need for the sake of pagination.

src/index.ts Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
src/helpers/url.ts Outdated Show resolved Hide resolved
src/helpers/url.ts Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/helpers/url.ts Outdated Show resolved Hide resolved
Comment on lines +3 to +5
type Query = {
[index: string]: string | number | boolean;
};
Copy link
Member

Choose a reason for hiding this comment

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

Could we replace all instances of this with URLSearchParams?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite, because URLSearchParams is not a plain object but a class instance. So we'd have to upgrade our query arguments created in our methods to actually be new URLSearchParams(query). It's a bit too involved of a change so I'd rather leave that out for later.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I was suggesting using URLSearchParams everywhere, rather than try to maintain two representations of the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I've added it to #145 to tackle post-adding the response types.

...paginationParams
}: CollectionId & PaginationParams & OrientationParam) => ({
pathname: `${COLLECTIONS_PATH_PREFIX}/${collectionId}/photos`,
query: compactDefined({ ...Query.getFeedParams(paginationParams), orientation }),
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to use compactDefined here? I thought you decided to do that inside createRequestHandler?

Also applies to other places in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry. I ended up tightening the Query type to not allow any non-values after all, which required me to add compactDefined wherever it was needed in methods. We no longer need to compact in createRequestHandler, so I removed it from there just now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants