New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for multi-asset fetching #170
Conversation
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.
LGMT, commented something about the longest path algo
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.
Looks good. Check also the error message on line 351, in the asset by version request, that was not updated in this PR, there seems to be 'alias'
instead of 'version'
:
if (!version) {
throw new Error('Missing mandatory parameter `alias`');
}
a560531
to
425f2ef
Compare
return `${path}/`; | ||
} | ||
|
||
const { length: l, 0: first, [l - 1]: last } = sortedPathParts; |
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.
Whou I did not know you can do destructions like this. TIL!
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.
Just learned it also while I was doing this 😀
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 love that typo / autocorrect :)
425f2ef
to
7cb7731
Compare
ede9618
to
ff5fb9f
Compare
Two new SDK fns: assetsByAlias() and assetsByVersion().
ff5fb9f
to
ac71532
Compare
dfbb0ad
to
37b5dae
Compare
Don't merge until release and keep the branch on top of master.
TODO:
Two new SDK fns: assetsByAlias() and assetsByVersion().