Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Fix cache policy for search exports query#43344

Merged
philipp-spiess merged 1 commit into
mainfrom
ps/fix-search-export-query
Oct 24, 2022
Merged

Fix cache policy for search exports query#43344
philipp-spiess merged 1 commit into
mainfrom
ps/fix-search-export-query

Conversation

@philipp-spiess

@philipp-spiess philipp-spiess commented Oct 24, 2022

Copy link
Copy Markdown
Contributor

Fixes #43222

Apparently, the default cache setup in Apollo can lead to properties being garbage collected before the onCompleted callback is invoked.

I have to investigate further to understand what exactly is going on but the symptom was that Apollo has removed fields from the resulting objects before the search export logic was able to read those values.

Thanks to @umpox for providing the hint regarding this having to do with the fetch policy.

Test plan

I verified locally that exporting search results indeed works again.

test.plan.mov

App preview:

Check out the client app preview documentation to learn more.

@philipp-spiess philipp-spiess requested review from a team October 24, 2022 09:16
@philipp-spiess philipp-spiess self-assigned this Oct 24, 2022
@cla-bot cla-bot Bot added the cla-signed label Oct 24, 2022
@sourcegraph-bot

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 30b15a0...b30670b.

Notify File(s)
@fkling client/web/src/search/results/useExportSearchResultsQuery.ts
@limitedmage client/web/src/search/results/useExportSearchResultsQuery.ts

@sg-e2e-regression-test-bob

Copy link
Copy Markdown

Bundle size report 📦

Initial size Total size Async size Modules
0.00% (0.00 kb) 0.00% (+0.02 kb) 0.00% (+0.02 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits b30670b and fc19858 or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

@vdavid vdavid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! My first thought was to see how disabling caching affects performance, but this is a feature that's not likely to be called twice, so this seems to be a fine solution. (I also saw that we use fetchPolicy: 'no-cache' at many places in the code base.)

@philipp-spiess philipp-spiess merged commit 075e902 into main Oct 24, 2022
@philipp-spiess philipp-spiess deleted the ps/fix-search-export-query branch October 24, 2022 09:50
@justdueck justdueck mentioned this pull request Nov 3, 2022
11 tasks
philipp-spiess added a commit that referenced this pull request Nov 8, 2022
philipp-spiess added a commit that referenced this pull request Nov 8, 2022
philipp-spiess added a commit that referenced this pull request Nov 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Export search results not working on dotcom

4 participants