feat(redirect): await submit for pending requests

[drodriguln]

Summary

There is a CR ticket that has been sitting for a while. This ticket describes customers who are using our redirect url plugin being able to submit what should end up being a redirect before the last request can come in and handle it. We've had similar issues appear in the past, but has previously tried to shift the responsibility onto the customer at the time as it seemed like an edge case, but this recent ticket has "tipped the scale" against that idea.

Result

There is a timing issue at play here especially for slow network connections. This PR aims to allow the redirect plugin to essentially await all existing network requests before submitting the form and subsequently handling any redirect data available from the last request. This required changes to the core library, rather than the plugin itself.

EDIT: changed the notes below based on initial feedback to set on the plugin itself rather than expose an option.

• Adds the awaitSubmit to the redirect url plugin
• Awaits pending requests for the submit handler in getPropGetters.ts if plugin?.awaitSubmit === true
• Prevents cancelling pending requests in onKeyDown.ts for the "Enter" key if plugin?.awaitSubmit === true
• Adds the async method store.pendingRequests.wait() to await all pending requests

1. Run the redirect url plugin playground
2. Enable network throttling to "Slow 3G"
3. Quickly type "algolia" and press enter
4. The input dropdown list should remain open while the pending requests come in until finally a redirect item is visible and the input closes and redirects to the Algolia main page
5. Repeat without throttling to ensure existing behavior still works correctly
6. Given this changes the core library, it would be good to check another plugin still operates correctly via its playground, etc

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0e030fc:

Sandbox	Source
@algolia/autocomplete-example-github-repositories-custom-plugin	Configuration
@algolia/autocomplete-example-instantsearch	Configuration
@algolia/autocomplete-example-playground	Configuration
@algolia/autocomplete-example-preview-panel-in-modal	Configuration
@algolia/autocomplete-example-starter-algolia	Configuration
@algolia/autocomplete-example-starter	Configuration
@algolia/autocomplete-example-reshape	Configuration
@algolia/autocomplete-example-vue	Configuration

[Haroenv]

I think this solution is the way to go, although I think the way the option is passed down can be improved. Ideally something that isn't based on an option but that can be read from the plugin options and not __options. Thanks for the PR!

[Haroenv]

this should be something set in the plugin, like plugin.awaitSubmit directly, so it's not based on the option

[drodriguln]

Yeah, that makes sense.

[sarahdayan]

We might actually want to change that. To me this should be an option controllable by the customer, not an immutable behavior of the plugin.

[sarahdayan]

Thanks for raising this issue @drodriguln, I understand we're in a difficult situation here because the nature of the redirect plugin changes the behavior of the autocomplete based on the response data, where everything else is mostly about displayed results, so it matters less when it's slow.

My issue with this proposed change is that it creates an odd behavior UX-wise. You submit, nothing happens, then finally everything resolves and it redirects you. This feels unexpected because the connexion between the user action and the redirect behavior is "lost". But also, if you typed a query that doesn't have a redirect, you're just blocked for potential redirections which in this case wouldn't exist. I don't know how many redirects customers have, but I anticipate that most queries don't have redirects attached to them.

Before we move forward with this or start thinking about mitigating with changes, I'd love to know (on Slack since this is private information):

• How many customers use redirects?
• Among those customers, how many queries have attached redirects vs. the total of their submitted queries?

Based on that data, we might be able to find a middle ground for customers who really want this, without affecting those who don't find this too important, because this is an important change.

[drodriguln]

@sarahdayan

My issue with this proposed change is that it creates an odd behavior UX-wise. You submit, nothing happens, then finally everything resolves and it redirects you.

I don't disagree, but customers have complained specifically about their users not always getting redirected. We have tried to steer them towards developing their own custom solution, but increasingly this timing issue is shown to be a flaw in the logic that needs to be addressed. And there's not much that can be done without providing some way to hook into the pending requests. That's something we could focus on doing instead to allow customers to allow developing custom fixes instead, but I'm open to other suggestions as I'm not well-versed in this codebase.

Based on that data, we might be able to find a middle ground for customers who really want this, without affecting those who don't find this too important, because this is an important change.

I could change this back to an option and make it opt-in only (default to being disabled)? Either way, currently this await behavior only occurs if a plugin has awaitSubmit: true, which only the redirect plugin can set here.

How many customers use redirects? Among those customers, how many queries have attached redirects vs. the total of their submitted queries?

I don't have this data off-hand, but we've had a few CR tickets come in since the plugin released complaining about this, including the one I have linked in the description. Nothing widespread, but fair reported criticisms. Given that, I'm certainly open to ideas for how to proceed.

[sarahdayan]

Hey @drodriguln, the change I'd have in mind is:

• Make this an option of the Redirect plugin.
• The option can be a predicate that either returns a boolean or a number, to indicate a timeout beyond which to move forward with the submit even if all requests aren't resolved. Making it a predicate instead of a static value would also allow customers to use the Network Information API and customize behavior based on network quality.
• By default, the value would be () => false so it doesn't affect customers who are fine with the current behavior.

That could look something like this:

autocomplete({
  plugins: [
    createRedirectUrlPlugin({
      awaitSubmitUntilResponse: () => true,
    }),
  ],
  // …
});

With a timeout:

autocomplete({
  plugins: [
    createRedirectUrlPlugin({
      awaitSubmitUntilResponse: () => 5000,
    }),
  ],
  // …
});

With the Network Information API:

autocomplete({
  plugins: [
    createRedirectUrlPlugin({
      awaitSubmitUntilResponse() {
        if (navigator.connection.effectiveType === '4g') {
          return false;
        }

        return 5000;
      },
    }),
  ],
  // …
});

cc @Haroenv

[drodriguln]

Hey @drodriguln, the change I'd have in mind is:

• Make this an option of the Redirect plugin.
• The option can be a predicate that either returns a boolean or a number, to indicate a timeout beyond which to move forward with the submit even if all requests aren't resolved. Making it a predicate instead of a static value would also allow customers to use the Network Information API and customize behavior based on network quality.

Hey @sarahdayan. That's a great idea. Sorry it took me a bit of time to get back to this, but I've just recently pushed up some commits to accomplish what you've requested. Please let me know if I've missed anything or you'd like to see some changes (outside of unit tests, which I'll take care of with another commit).

Also, side note: the bundlesize CI tasks are failing for core, but I haven't added that much more code. Any clue why this is failing? If it really is from what I've added, should I bump up the threshold?

[shaejaz]

I might be testing this incorrectly, but after I throttle my network, quickly type and press enter, the dropdown remains open until the redirect link shows up but it's not actually redirecting it to the URL in the end.
Maybe I have too large of a await number? I'm throttling it to like 50kbs to make it really delayed. I'm also testing it on the redirect example and not the playground to make sure it's nothing else interfering.

[dhayab]

I like the latest implementation 👍.

Also, side note: the bundlesize CI tasks are failing for core, but I haven't added that much more code. Any clue why this is failing? If it really is from what I've added, should I bump up the threshold?

You can slightly bump the threshold for core/umd and js/umd, I'm not worried about the difference in size.

[dhayab]

Suggested change

	describe.skip('onSubmit', () => {
	describe('onSubmit', () => {

[dhayab]

Suggested change

	describe.skip('a plugin is configured with the option "awaitSubmit"', () => {
	describe('a plugin is configured with the option "awaitSubmit"', () => {

[drodriguln]

I might be testing this incorrectly, but after I throttle my network, quickly type and press enter, the dropdown remains open until the redirect link shows up but it's not actually redirecting it to the URL in the end. Maybe I have too large of a await number? I'm throttling it to like 50kbs to make it really delayed. I'm also testing it on the redirect example and not the playground to make sure it's nothing else interfering.

There appears to be some kind of race condition where the last results do not show a redirect item (it will appear and then disappear). I'm assuming this is the issue you're seeing. My theory is the requests are not guaranteed to be resolved in the same order they are received so the last one is not always the one with the redirect. I'll look more into this. If not that, then maybe I missed handling a submit event somewhere.

other issues showed