PURCHASE-1599: Consolidates Auction State across Artwork and Bid pages

sweir27 commented 5 years ago

This PR addresses some issues we were seeing with the auction state transitions.

The immediate changes included are:

Removes our custom AuctionCountdownTimer component in favor of re-using the auction state handling and countdown timer that already exists in the bidding flow. This simplified the logic within the artwork view significantly.
Removes relay caching on the artwork view. The state of the artwork (and in particular the corresponding sale which is nested within) can change often during the course of an auction, so we need to be able to see the updated state immediately upon refreshing the view. cc @alloy in case you have another idea

A few things that are not necessarily ideal included in here:

The AuctionCountdownTimer (removed here) used luxon instead of moment. As I am re-using the existing bid flow timer here (which uses moment), I did not refactor to remove that. This should probably be a standalone follow-up as it'll also affect the fair countdown timer.
The DurationProvider wrapper provides a nice abstraction around countdowns, but it means that the view it encompasses is re-rendered every second. I didn't notice any major issues in the simulator, but it's something to watch.
The existing timer also uses an Emission-specific component instead of the palette-backed TimeRemaining, simply because that would also be a significant refactor.

One outstanding issue related to caching: Metaphysics' current caching scheme means that while we don't cache the response of the Artwork, we do cache the response of the Sale that's returned from within the artwork.

This creates the following situation:

A live auction integration sale ends --> The corresponding Artwork is marked as "Not for sale" --> The corresponding Sale is marked as "Closed"
The artwork view won't know that the auction has closed until Metaphysics' cache is busted, meaning we get a weird in-between state until that happens

This ☝️ is also an issue on force, although the UI handles that by just showing the "Enter Live Bidding" button for an extra minute or so after the live auction has technically closed. We should follow up on this one.

All of the states!

Live

PREVIEW

wee-preview-start-live

BEFORE LAI

wee-start-LAI-live

DURING LAI

Online-Only

PREVIEW

wee-preview-start-oo

OPEN

wee-start-end-oo

CLOSED

kierangillen commented 5 years ago

Nice refactor!! LGTM!

kierangillen commented 5 years ago

Does this entity header image look squished?

sweir27 commented 5 years ago

@alloy Not all artworks are in sales. Both of your proposed solutions sound okay, I just wasn't sure how to accomplish them. Can you point me in the right direction?

sweir27 commented 5 years ago

@kierangillen the entity header does look squashed 😞 appears to be happening on master as well for this artist.

joeyAghion commented 5 years ago

@sweir27 so you're seeing the staleness we anticipated in https://artsyproduct.atlassian.net/browse/PLATFORM-1309 in the wild? If so, seems like we should implement what was discussed in that ticket and bypass typical caching for sales (or perhaps selectively, like for authenticated callers or sales that aren't yet closed). I wasn't racing to do that since I didn't realize it was a confirmed problem, but this changes things.

mzikherman commented 5 years ago

I don't fully follow the cache staleness issue. Is it that once a sale state is updated, which should be reflected in the UI, that takes 60 seconds for the MP cache to clear, and thus the UI might show a link to a live sale, for up to 60 seconds after it's over?

If so, that might be behavior introduced when we switched MP caching away from the 'eagerly warming the cache' to a more standard one. I can imagine the former being faster to update.

That cache is configurable, so we might be able to set it better for sales being requested for an artwork, or whatever.

sweir27 commented 5 years ago

@mzikherman That's right!

alloy commented 5 years ago

If so, that might be behavior introduced when we switched MP caching away from the 'eagerly warming the cache' to a more standard one. I can imagine the former being faster to update.

Perhaps both apply or we are conflating things, but I understood this to be about the cache-config force parameter that’s passed to Relay when performing the operation, which would make it ignore the response cache it has for the given operation+variables.

alloy commented 5 years ago

@alloy Not all artworks are in sales. Both of your proposed solutions sound okay, I just wasn't sure how to accomplish them. Can you point me in the right direction?

Would it be feasible, if not already the case, to wrap all of the sale related containers in a single refetch container? If so, then that refetch container could force-fetch after loading the data from the response cache, meaning you can either:

optimistically show the entirety of the artwork view from cache and update the sale part when the up-to-date data comes in; or
pessimistically only show the non-sale parts of the UI until the up-to-date comes in.

The former could perhaps also be achieved in a simpler way by using the store-and-network fetch policy for the operation, except that it would also re-fetch all of the non-sale data. Unsure, though, as ‘the store’ != ‘response cache’, so we'd need to experiment. See https://relay.dev/docs/en/query-renderer

sweir27 commented 5 years ago

@alloy I figured out that when refetching, we were getting new data from metaphysics, so I updated the code to make sure it was handling that okay (the StateManager didn't handle new props after the initial render).

It also seems that the caching issue is scoped to Emission's example app, since we already have handling for re-fetching data when someone pulls to refresh or the view becomes visible. We can still make the change you're suggesting but I don't think it should block this work.

I didn't make the TypeScript changes you suggested because I didn't have time, but I'm happy to follow up with you on that.

alloy commented 4 years ago

We can still make the [caching] change you're suggesting but I don't think it should block this work.

Agreed, and as discussed the way to achieve it (by first rendering from cache and then updating from network) would require some dedicated research work, applies to all of [future] Emission, and is very UX related; so I think it makes sense for the Mobile Experience team to take that on. For instance, I know that @ds300 wants to eliminate spinners as much as possible. /cc @ashfurrow

I didn't make the TypeScript changes you suggested because I didn't have time, but I'm happy to follow up with you on that.

Let’s please do that 👍

artsyit commented 4 years ago

:rocket: PR was released in v1.18.10 :rocket:

artsy / emission