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

feat: Statically Cache Marketplace Apps #9732

Conversation

bnussman-akamai
Copy link
Member

Description 📝

  • Statically cache Marketplace Apps and use them as the initialData for React Query

Major Changes 🔄

  • Use wrapper instead of manual fetch for class component
  • Add caching

Preview 📷

Before After
Screen.Recording.2023-09-28.at.4.56.05.PM.mov
Screen.Recording.2023-09-28.at.4.55.44.PM.mov

How to test 🧪

  • Check performance and functionality of http://localhost:3000/linodes/create?type=One-Click

@bnussman-akamai bnussman-akamai self-assigned this Sep 28, 2023
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner September 28, 2023 20:57
@bnussman-akamai bnussman-akamai requested review from mjac0bs and carrillo-erik and removed request for a team September 28, 2023 20:57
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for the POC! I'll spend more time with it but I had a couple initial questions

packages/manager/src/containers/withMarketplaceApps.ts Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

now I know why we get those at build time! speaking of, are we supposed to always commit updates when we seen them? for example, regions seems to be an exception cause it does not contain some regions we have been working with (Jakarta, Sao Paulo)?

Copy link
Member Author

@bnussman-akamai bnussman-akamai Sep 29, 2023

Choose a reason for hiding this comment

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

I'm not sure why regions.json contains Jakarta and Sao Paulo. Maybe we manually added it so we could work on DC specific pricing easier? In my opinion, we shouldn't use really serve regions.json from the MSW and we should just use factories instead.

We talked about .gitignoreing these JSON files to prevent misuse like this of caching

},
{ '+order_by': 'ordinal' },
],
},
Copy link
Contributor

Choose a reason for hiding this comment

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

so this will work for all stackscripts requests? (stackscripts + OCA)? Or just OCA because of the filter definition?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just Marketplace. This filter is a copy-paste of oneClickFilter (I don't think I can import it from TS into a JS file)

I don't think we want to attempt StackScript caching because at that point we'd have to worry about pagination.

@carrillo-erik
Copy link
Contributor

The user experience is much better, the old way definitely took too long. I'm taking a closer look at the code to familiarize myself with this feature a bit more.

Copy link
Contributor

@abailly-akamai abailly-akamai 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 really cool. As pointed out before, it more of a cold start data filler than a cache

The only case where a user could notice stale data is if they

  • try to select a app that was added in the API response before a rebuild occurred
  • try to select that app before the real request actually completed in the background and replaced the cold cache.

Those cases are pretty edgy considering we need a release for every new OCA.

I see no downsides to implementing this, and marketplace is a perfect case for using it.

  • data loads immediately ✅
  • requests happens in the background ✅
  • data gets updated once request completed ✅
  • e2e still passes

This is a super nice improvement 👏

🧪 I modified marketplace.json to not have any record and the content updates once the request completes in the background.

Screen.Recording.2023-10-02.at.14.56.25.mov

@abailly-akamai
Copy link
Contributor

@bnussman I propose removing POC from the title and a changeset to get this into the next release

@bnussman-akamai bnussman-akamai changed the title poc: Statically Cache Marketplace Apps feat: Statically Cache Marketplace Apps Oct 3, 2023
Copy link
Contributor

@mjac0bs mjac0bs 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 definitely a user experience improvement when loading that page.

  • The unit test failure can be fixed now with the merging of develop.
  • Can we create a corresponding M3 ticket for this so it is tracked?

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

Successfully merging this pull request may close these issues.

5 participants