-
Notifications
You must be signed in to change notification settings - Fork 332
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
change: [M3-7802] - Remove API caching #10219
change: [M3-7802] - Remove API caching #10219
Conversation
Coverage Report: β
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! this is a nice cleanup π§Ή
As a result this API update also improved the stackscripts/community tab which was still painfully slow. π
Noticed no regression β
Approving pending no e2e complaints β
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great clean up π§Ό. Approved with one question.
server.use( | ||
rest.get('*/regions', (req, res, ctx) => { | ||
const regions = regionFactory.buildList(1, { | ||
id: 'us-central', | ||
label: 'Fake Region, NC', | ||
}); | ||
return res(ctx.json(makeResourcePage(regions))); | ||
}) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I fully understand why these mocks had to be added. Is the mock from serverHandlers.ts
incorrect (I believe it comes from regionsData
)? If so, should they be changed/removed from that file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mocks in serverHandlers.ts
are the default, but in my opinion, it's best practice to explicitly define any API data that the component depends on in the test itself. This way the unit test will always pass, even if we make changes in serverHandlers.ts
, which is something we do frequently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the cleanup @bnussman
Verified and no regression found.
Description π
Kernel
factoryKernel
type to match what the API actually returnsWhy β
yarn build
without an internet connection because we no longer need to fetch from the API πWhat will be removed and why βΉοΈ
src/cachedData/typesLegacy.json
because it was unusedsrc/cachedData/kernels.json
because it was only used for unit testing (I replaced it with a factory π)src/cachedData/marketplace.json
because the API now responds much faster (~300ms) (use to take many seconds β²οΈ )src/cachedData/regions.json
because it was an over-optimization, not gitignored, and was misused as mock data - see upcoming: [M3-7756] - Placement Groups limits updatesΒ #10191 (comment) (this was recently corrected by @abailly-akamai but I don't want it to happen again in the future) πHow to test π§ͺ
As an Author I have considered π€