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

fix: axios cancel alternative not working #236

Merged
merged 9 commits into from
Oct 17, 2023
Merged

Conversation

alfonsobries
Copy link
Contributor

@alfonsobries alfonsobries commented Oct 16, 2023

Summary

Closes https://app.clickup.com/t/865d85uzc

Replaces the globally added abort controller with a different solution (how to test below),

Also with the new fix in place I noticed that the collection page was making two requests to load the collections when opened, meaning double memory usage, also was requesting the collection for guest users, fixed both issues

To test the axios cancel part you need to emulate a canceled request, that is mostly used on the search fields, so for example if a user is creating a gallery and searches for a nft, then changes the search query but the prev search request didnt finished yet, it should cancel the prev request, since is not easy to emulate that scenario you can hardcode a sleep method in the controller that return the response, for example, on the app/Http/Controllers/MyGalleryCollectionController.php you can add sleep(5) inside the index method. since request is going to be slower if will be canceled while you search (see video)

image

Screen.Recording.2023-10-13.at.16.06.59.mov

This apply to every component that I updated with newAbortSignal, is not easy to explain how to test all of them but if you have issues ping me on slack,

Checklist

  • I checked my UI changes against the design and there are no notable differences
  • I checked my UI changes for any responsiveness issues
  • I checked my (code) changes for obvious issues, debug statements and commented code
  • I opened a corresponding card on Clickup for any remaining TODOs in my code
  • I added a short description on how to test this PR (if necessary)
  • I added a storybook entry for the component that was added (if necessary)
  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

let data: CollectionsResponse;

try {
const response = await axios.get<CollectionsResponse>(decodeURIComponent(url.toString()), {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 🔥 🚀

Copy link
Contributor

@patricio0312rev patricio0312rev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is lit 🔥

@ItsANameToo ItsANameToo merged commit a10d997 into develop Oct 17, 2023
16 checks passed
@ItsANameToo ItsANameToo deleted the fix/axios-cancel branch October 17, 2023 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants