elastic / app-search-javascript

Elastic App Search Official JavaScript Client
https://www.elastic.co/products/app-search
Apache License 2.0
66 stars 21 forks source link

Allow GET method for Queries to App Search API #23

Closed byronhulcher closed 4 years ago

byronhulcher commented 4 years ago

The App Search documentation states that one can use a GET or POST request when accessing /api/as/v1/engines/{ENGINE_NAME}/search for a search query. However, app-search-javascript only allows you to utilize a POST request for this.

POST requests are meant for unique non-repeatable actions, while GET requests are intended for repeatable actions without side effects. An example of how this plays out in the real world is if you submit a POST request on page load, then attempt to refresh the page, Google Chrome will warn you with an alert pop-up asking you to "Conform Form Resubmission".

The above example can occur in @elastic/search-ui when utilizing "alwaysSearchOnInitialLoad: true". Because @elastic/search-ui utilizes @elastic/app-search-javascript as a dep, and @elastic/app-search-javascript uses POST requests by default, when alwaysSearchOnInitialLoad: true then the page will fire a POST request, and attempting to refresh the page or navigate back to it will trigger the "Confirm Form Resubmission" alert, even though the user took no action.

We should supply an option so that people utilizing the @elastic/app-search-javascript library can make the choice for themselves which method to use, based on their implementation requirements.

byronhulcher commented 4 years ago

Hey @kovyrin @JasonStoltz I think you two might be the most knowledgable about our integrations. I'd like to get a convo going if it would be possible to execute this update to app-search-javascript as a non-breaking change (IE, we don't change the function signature). The change itself seems very small I'm just not sure the correct way we'd like to see it implemented.

Alternatively, if we think it would be a breaking change requiring a major version release, I'd like for us to determine what that changes would be specifically, and understand what if anything would block us from immediately implementing and publishing it.

Just trying to get a sense of "what changes in code specifically should be done to address this" and "how soon after making those changes will I be able to publish them"

JasonStoltz commented 4 years ago

@byronhulcher I don't understand why this change is needed? Since this is an XHR call, it shouldn't matter whether we use POST or GET.

byronhulcher commented 4 years ago

Based on conversation in Slack, this is downgraded from "urgent request" to "nice to have feature". I still think we should do it (so app-search-javascript matches the API) but doesn't need to be immediate.

JasonStoltz commented 4 years ago

Hi @byronhulcher, I disagree that this is useful. Why would someone care whether it is a GET or a POST? Unless there is a compelling reason, I'd like to close this issue.

JasonStoltz commented 4 years ago

Whether it is a GET or a POST is essentially an implementation detail that shouldn't concern the user of this client, IMO.

byronhulcher commented 4 years ago

I'm fine closing this, it's looking like my original issue is not related to this and there isn't a need to provide both methods at this time.

richkuz commented 4 years ago

Thanks. Just to add more background, in case someone else stumbles across this:

In the originally reported use case, we used the app-search-javascript client to POST a query via XHR. After querying, when clicking the Back button or refreshing the results page, the browser displayed an undesired "Confirm Form Resubmission" dialog. Upon further investigation, we found that the Confirm dialog was not being triggered by the app-search-javascript POST XHR request. Rather, an unrelated form POST on a page several steps prior was causing all pages to display the Confirm dialog upon page refresh.

We value having the smallest possible (useful) JS API footprint. Adding an API wrapper around the GET API would be an implementation detail with questionable value for end users. If someone comes upon this ticket and has a new use case we haven't considered, please reopen this issue and state the use case so we can reconsider.