-
Notifications
You must be signed in to change notification settings - Fork 160
Feature/typescript #143
Feature/typescript #143
Conversation
use pipe
lukechesser
left a comment
There was a problem hiding this 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?
There's more! It's pretty much all changed. I'll need to write up a new readme of course. Biggest things:
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 |
…into feature/typescript
…ne helpers" This reverts commit bc34062.
| type Query = { | ||
| [index: string]: string | number | boolean; | ||
| }; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 }), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Co-authored-by: Oliver Joseph Ash <[email protected]>
Co-authored-by: Oliver Joseph Ash <[email protected]>
Overview
TO-DO
Languageand string literal unions for args)urlmodule