Move /shorten to /api/shorten_url #21808
Conversation
💚 Build Succeeded |
|
Nice! Couple things: Can you add a section for this API under @epixa - what's the standard way to highlight a deprecation in the rest API? No documentation around this API currently exists in asciidoc. I think that's just because we don't have a lot of documentation, not for any other reason. So there isn't an existing |
| const shortUrlLookup = shortUrlLookupProvider(server); | ||
|
|
||
| server.route(createGotoRoute({ server, config, handleShortUrlError, shortUrlAssertValid, shortUrlLookup })); | ||
| server.route(createShortenUrlRoute({ handleShortUrlError, shortUrlAssertValid, shortUrlLookup })); |
There was a problem hiding this comment.
Why pass shortUrlAssertValid in as a parameter rather than import directly from those files? Same with handleShortUrlError.
There was a problem hiding this comment.
no real reason. I was passing in shortUrlLookup so just figured to pass in everything to stay consistent.
There was a problem hiding this comment.
Do you ever see these functions needing to handle different implementations of those two parameters? If not, I think it'd be more consistent and less complex if you imported and called directly. The fewer function parameters you can get away with, the simpler the code (and easier to test).
|
issue to update breaking changes log has been created #21836 |
|
We should do the breaking change doc in the same PR that actually removes the existing behavior in master. That PR can be thrown up as soon as this is merged rather than having a lingering blocker for 7.0.
There is no such standard. Without specific documentation for this endpoint, we should just mention the change in the release notes and make sure there's an appropriately documented breaking change in master. |
| @@ -144,9 +143,12 @@ app.directive('share', function (Private) { | |||
| this.urlFlags.shortSnapshot = !this.urlFlags.shortSnapshot; | |||
|
|
|||
| if (this.urlFlags.shortSnapshot) { | |||
| urlShortener.shortenUrl(this.urls.snapshot) | |||
| shortenUrl(this.urls.snapshot) | |||
There was a problem hiding this comment.
I kind of thought we decided to cast es6 promises to angular promises, rather than calling $scope.$apply()?
Promise.resolve(shortenUrl(this.urls.snapshot))
.then(shortUrl => {
this.urls.shortSnapshot = shortUrl;
})There was a problem hiding this comment.
we could but it will not matter in the next couple of days since the next step for EUI and rectifying the share top nav is to rip this directive out replace with a react component. This is just temporary.
There was a problem hiding this comment.
Just for other efforts similar to this, if we can avoid using $scope.$apply() we should.
| }); | ||
| }).catch((response) => { | ||
| notify.error(response); | ||
| return kfetch({ method: 'POST', 'pathname': '/api/shorten_url', body }).then((resp) => { |
There was a problem hiding this comment.
if that is preferred. Figured with the catch block this was just as easy.
There was a problem hiding this comment.
I think async/await is more than just easy, I think it's also easier to read and more aligned with non-async code, so I would say I prefer it regardless.
💚 Build Succeeded |
💚 Build Succeeded |
* Move /shorten to /api/shorten_url * better API test assertions * add API documenation * use async await * import dependencies instead of pass in
This reverts commit 803a3f1.
prerequisite for migrating share top nav to EUI and react. The reason for such a big PR is that
kfetchexpects the response to be JSON and/shortenreturns a string.This PR
url_shortener/api/shorten_urlthat is a replacement for the existing endpoint/shorten./shortendoes not return JSON and is not under/api./shortenas deprecated so it can be removed in 7.0