prismic-client icon indicating copy to clipboard operation
prismic-client copied to clipboard

feat: optimize concurrent queries via shared network requests

Open angeloashmore opened this issue 3 years ago • 2 comments

Types of changes

  • [ ] Chore (a non-breaking change which is related to package maintenance)
  • [ ] Bug fix (a non-breaking change which fixes an issue)
  • [x] New feature (a non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

To do before merging

  • [ ] Add tests for all methods using Client.prototype.fetch()
  • [ ] Refactor code to reduce size increase

Description

This PR optimizes concurrent queries by sharing network requests. If two or more concurrent network requests look identical (same URL and, if given, signal), only one network request is made.

The primary purpose of this optimization is to share calls to /api/v2 (i.e. getCachedRepository()), which all queries perform before calling the /api/v2/documents/search endpoint. The implementation, however, allows any concurrent equivalent network requests to be shared.

The following contrived example demonstrates identical concurrent queries:

import * as prismic from "@prismicio/client";

const client = prismic.createClient("qwerty");

// Two network requests are made:
//
// 1x request to `/api/v2`
// 1x request to `/api/v2/documents/search`
await Promise.all([
  client.get(),
  client.get(),
  client.get(),
  client.get(),
  client.get(),
]);

The following more realistic example demonstrates practical concurrent queries:

// Four network requests are made:
//
// 1x request to `/api/v2`
// 3x requests to `/api/v2/documents/search`
await Promise.all([
  client.getByUID("page", "about"),
  client.getSingle("navigation"),
  client.getSingle("settings"),
]);

Note how only one call to /api/v2 is made. Without this PR, multiple concurrent calls to /api/v2 are necessary since each query starts immediately without a cached /api/v2 response.

// Without this PR, six network requests are made:
//
// 3x request to `/api/v2`
// 3x requests to `/api/v2/documents/search`
await Promise.all([
  client.getByUID("page", "about"),
  client.getSingle("navigation"),
  client.getSingle("settings"),
]);

Abortable queries via AbortSignal

All query methods are abortable (i.e. cancelable) via the signal parameter. An AbortSignal, usually controlled by an AbortController, can be aborted, causing the network request to be stopped and throw an error.

A query with a signal and an equivalent query without a signal are not equivalent. If the AbortSignal is cancelled, we don't want the signal-less query to abort as well. Thus, network requests cannot be shared.

const controller = new AbortController();

// Four network requests are made:
//
// 2x request to `/api/v2`
// 2x requests to `/api/v2/documents/search`
await Promise.all([
  // Shared
  client.get(),
  client.get(),

  // Not shared
  client.get({ signal: controller.signal }),
]);

Things to consider

This approach to implementing #260 provides optimizations that are general. All network requests, not just calls to getCachedRepository(), are optimized.

Using a general implementation may not be necessary or useful in real-world scenarios. Calling the same query within a single Promise.all() should almost never happen.

It is possible for a single client to be shared across multiple contexts which makes identical queries in quick succession, such as a static site generator that calls client.getSingle("settings") on each page.

The general implementation in this PR takes up more bytes than something specific to getCachedRepository(). We need to decide if the bytes are worth optimizing every, albeit infrequent, edge case.

Closes #260

Checklist:

  • [ ] My change requires an update to the official documentation.
  • [x] All TSDoc comments are up-to-date and new ones have been added where necessary.
  • [x] All new and existing tests are passing.

angeloashmore avatar Aug 27 '22 04:08 angeloashmore

size-limit report 📦

Path Size
dist/index.js 4.26 KB (+2.74% 🔺)
dist/index.cjs 7.17 KB (+1.53% 🔺)

github-actions[bot] avatar Aug 27 '22 04:08 github-actions[bot]

Codecov Report

Merging #261 (3fcdb42) into v7 (a520aa4) will increase coverage by 0.00%. The diff coverage is 100.00%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##               v7     #261   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files          48       48           
  Lines        5281     5326   +45     
  Branches      303      311    +8     
=======================================
+ Hits         5279     5324   +45     
  Misses          2        2           
Impacted Files Coverage Δ
src/createClient.ts 99.94% <100.00%> (+<0.01%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov-commenter avatar Aug 27 '22 04:08 codecov-commenter

Do we want to still work on that or no? I feel like it'd still be beneficial

lihbr avatar Jan 20 '23 10:01 lihbr

Yes, let's try to get this into v7. Sorry, it's my fault that this didn't proceed; I totally forgot about it! I'll finish this PR before we merge v7.

angeloashmore avatar Jan 23 '23 20:01 angeloashmore

No pressure, but any update on this @angeloashmore?

lihbr avatar Mar 14 '23 11:03 lihbr

All tests have been updated with a new testConcurrentMethod() test helper.

I messed up and made all the changes as part of the v7 merge commit. :<

The implementation has not been refactored to reduce the size increase, but I think we can leave it as is for now. As we get closer to a v7 release with its full feature set, we can refactor and optimize for size.

angeloashmore avatar Apr 11 '23 02:04 angeloashmore