-
Notifications
You must be signed in to change notification settings - Fork 160
Conversation
use pipe
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
Language
and string literal unions for args)url
module