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 "not found" messages before loading #550

Closed
3 tasks done
valiafetisov opened this issue Nov 28, 2022 · 7 comments · Fixed by #563
Closed
3 tasks done

Fix "not found" messages before loading #550

valiafetisov opened this issue Nov 28, 2022 · 7 comments · Fixed by #563
Assignees

Comments

@valiafetisov
Copy link
Contributor

valiafetisov commented Nov 28, 2022

Goal

Not found messages in the UI no longer displayed on load.

Context

Currently,

  • When a page to an auction is opened directly via url, the UI first show "This auction was not found" error and only later (a few seconds later) change it into the loading state
  • When the main page is opened with the table or auctions, we also first display "No active auctions" and only a few moments later show loading state

Instead, the pages should always show loading state first or somehow indicate proper state of the auction
Please note that we once had a similar problem that was previously fixed: #90 (comment)

Assets

Tasks

  • Come up with several approaches to fix it, choose the best (we also don't want to show loading when it's not actually loading yet)
  • Fix auction page error
  • Fix auction table error
@aomafarag aomafarag self-assigned this Nov 30, 2022
@aomafarag
Copy link
Contributor

aomafarag commented Dec 5, 2022

Since we always carry out a fetching attempt at least once when we open an auction, my approach would be:

  • Combine the fetching props in a computed prop:
isFetching(): boolean {
  return this.areAuctionsFetching || this.areTakeEventsFetching;
}
  • Add a new data prop to indicate whether an initial fetching attempt has taken place:
isFirstFetch: true;
  • Add a new watcher for isFetching
isFetching(newIsFetching) {
  if (newIsFetching) {
    this.isFirstFetch = false;
  }
}
  • Modify Loading component:
<Loading v-if="isFirstFetch || isFetching" />
  • Add !isFirstFetch as an extra condition for the auction not found error

@valiafetisov
Copy link
Contributor Author

Much more simpler approach would be to set areAuctionsFetching to true by default in the store, which would produce exactly the same result, right?

@aomafarag
Copy link
Contributor

Much more simpler approach would be to set areAuctionsFetching to true by default in the store, which would produce exactly the same result, right?

Well, that was my very first approach, but I refrained from it due to this point in the issue description:

  • Come up with several approaches to fix it, choose the best (we also don't want to show loading when it's not actually loading yet)

However, I don't think it would function the exact same way as it does not take areTakeEventsFetching into consideration. Setting both areAuctionsFetching and areTakeEventsFetching to true initially is a little sketchy imo.

@aomafarag
Copy link
Contributor

aomafarag commented Dec 7, 2022

I also have an improvement suggestion:

In store/network, we await each setup dispatch. My suggestion would be to run them all in parallel using Promise.allSettled(). This alone, btw, was a hacky pseudo-solution to the "not found" issue: the error message flashes for a few milliseconds (the user can't notice it).

@valiafetisov
Copy link
Contributor Author

as it does not take areTakeEventsFetching into consideration

Those are two sequential events. Currently, in the container, we wait for the areAuctionsFetching to finish, then check if something was fetched and if not, fetch events. This was a temporary and quick workaround since it should all happen in the core and hopefully later be refactored. So I don't want to overcomplicate this logic now, since it's also does not add any benefits

I also have an improvement suggestion

This is out of scope of this issue, as far as I understand. Promise.allSettled() can create some undesired side-effects (eg 429 error codes). Instead, fetching of all auction types should be improved by checking the page we're currently in and only firing related actions (and re-firing them when page changes). You can work on this in another PR/issue.

@aomafarag
Copy link
Contributor

Much more simpler approach would be to set areAuctionsFetching to true by default in the store, which would produce exactly the same result, right?

So, for now, I should go with this option for this issue, right?

@valiafetisov
Copy link
Contributor Author

Yes, I think it would be ok. You just need to make sure (by reading the code and testing it for all types of auctions/vaults) that we always do those requests and clear up them to false (inside finally)

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

Successfully merging a pull request may close this issue.

2 participants