Skip to content
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

Ability to prevent/cancel fetch on focus #31

Open
jdanyow opened this issue Jan 24, 2023 · 3 comments
Open

Ability to prevent/cancel fetch on focus #31

jdanyow opened this issue Jan 24, 2023 · 3 comments

Comments

@jdanyow
Copy link

jdanyow commented Jan 24, 2023

I'm using the remote input in a SelectMenu. The menu's SelectMenu-list is controlled by the remote input. When the page is rendered server-side, a few commonly used items are rendered in the SelectMenu-list to save the user time. The problem I have is these items are instantly cleared when the SelectMenu opens and the remote input receives focus due to the remote input doing the initial fetch.

Would the team be open to a PR that makes it possible to skip/cancel the initial fetch? I'd like to avoid the unnecessary round-trip to the server to get suggestions for the empty string query and preserve by server-side rendered default SelectMenu items. If yes, should it be done via html attribute or by preventing default on one of the custom events?

Two other questions:

  1. Should load fire before the fetch starts?
    const response = await fetch(url, options)
    el.dispatchEvent(new CustomEvent('load'))
    el.dispatchEvent(new CustomEvent('loadend'))
  2. Most github elements store state in a weakmap and use module functions (as opposed to class properties and methods). This makes it difficult to monkey patch component behavior. Is that the intent?

Thank you for sharing the github elements. They're super useful and I love the decoupling of the styles from the behavior.

@keithamus
Copy link
Member

keithamus commented Jan 25, 2023

Would the team be open to a PR that makes it possible to skip/cancel the initial fetch?

Yep sounds like a great addition 👍

  1. Should load fire before the fetch starts?

We try to follow the HTML Standard for this: https://html.spec.whatwg.org/#concept-media-load-algorithm. Media should be loaded roughly like this:

wait one "Task"
emit `loadstart`
fetch the data
wait one Task
if fetch caused error
  emit error
  emiit loadend
else
  emit load
  emit loadend

In this case it I believe load is executing at the right time. We also fire a loadstart event before the fetch - this is likely the event you want to target for the cancelable behaviour.

  1. Most github elements store state in a weakmap and use module functions (as opposed to class properties and methods). This makes it difficult to monkey patch component behavior. Is that the intent?

This is certainly intentional. We try to expose public interfaces with intentionality. Having said that this project pre-dates our use of Private Fields which, if you are undertaking a refactor, I would encourage you to use over WeakMaps as they do offer better ergonomics.

If you wish to expose some state to allow for monkey patching, we're happy to discuss ideas - please provide a reasonable use case, so we can explore what an API might look like.

@jdanyow
Copy link
Author

jdanyow commented Jan 30, 2023

Thanks for the detailed response! Based on this it sounds like making loadstart cancellable and including the url in it's details would be a good start.

With regards to monkey patching, I don't have an urgent need other than the use-case here- preventing the empty string fetch. Cancelling loadstart is much nicer than monkey patching. Another scenario might be customizing the fetch/response handling, eg when a 401 occurs, the application might want to show some UI or silently refresh the auth state and retry.

I'm having some trouble with the package deps. Will troubleshoot further when I have some more time.
CleanShot 2023-01-29 at 15 55 39@2x

@keithamus
Copy link
Member

Other elements we have assign a fetch function to themselves which allows for monkey patching (for example <include-fragment>). I'm not fully convinced about the pattern, however. I think having loadstart being cancelable would be better. I think we could also look to customize load to allow for customising the response data, but perhaps that's another discussion 😉

The deps is likely because this element hasn't been updated in a while. I'm not sure where graphql as a dependency comes from, but please feel free to submit a PR updating the dependencies, if you are able to. If not let me know and I can take a look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants