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(nuxt): add dedupe option for data fetching composables #24564

Merged
merged 21 commits into from Dec 14, 2023

Conversation

theguriev
Copy link
Contributor

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Added dedupe option to allow deduplicating requests to be used only once at the time. It is super useful in such cases:

<script setup lang="ts">
const { data, pending, error, refresh } = useAsyncData(
  "test",
  () =>
    new Promise((resolve) => {
      console.log("fetching data...");
      setTimeout(() => {
        resolve("Hello World");
      }, 1000);
    }), {
      dedupe: true
    }
);
useAsyncData(
  "test",
  () =>
    new Promise((resolve) => {
      console.log("fetching data...");
      setTimeout(() => {
        resolve("Hello World");
      }, 1000);
    }), {
      dedupe: true
    }
);
useAsyncData(
  "test",
  () =>
    new Promise((resolve) => {
      console.log("fetching data...");
      setTimeout(() => {
        resolve("Hello World");
      }, 1000);
    }), {
      dedupe: true
    }
);
useAsyncData(
  "test",
  () =>
    new Promise((resolve) => {
      console.log("fetching data...");
      setTimeout(() => {
        resolve("Hello World");
      }, 1000);
    }), {
      dedupe: true
    }
);
useAsyncData(
  "test",
  () =>
    new Promise((resolve) => {
      console.log("fetching data...");
      setTimeout(() => {
        resolve("Hello World");
      }, 1000);
    }), {
      dedupe: true
    }
);
useAsyncData(
  "test",
  () =>
    new Promise((resolve) => {
      console.log("fetching data...");
      setTimeout(() => {
        resolve("Hello World");
      }, 1000);
    }), {
      dedupe: true
    }
);
</script>

<template>
  <!-- Edit this file to play around with Nuxt but never commit changes! -->
  <div>Nuxt 3 Playground {{ data }}</div>
</template>

<style scoped></style>

I'm using something similar already in real prod project. But! I had to build another hook on top of that. It looks like this...

/* eslint-disable indent */

import { AsyncDataOptions } from "#app";

import { v1, ExtractFromAPI, PossibleMethods, Parameters as Params, replacePathParameters } from "@netgame/openapi";
import { NitroFetchOptions, NitroFetchRequest } from "nitropack";
import { hash } from "ohash";

import useAsync from "./useAsync";
import type { KeysOf } from "./useAsync";

export const dedupePromises = new Map();

const useAsyncFetch = <
	Path extends keyof v1.paths,
	Method extends keyof v1.paths[Path],
	Response extends ExtractFromAPI<v1.paths, Path, Method>,
	DataE = Error,
	DataT = Response,
	PickKeys extends KeysOf<DataT> = KeysOf<DataT>,
	DefaultT = null
>({
	path,
	method,
	options,
	fetchOptions
}: {
	path: Path;
	method: Method;
	options?: AsyncDataOptions<Response, DataT, PickKeys, DefaultT> & { cached?: true; key?: string };
	fetchOptions?:
		| (NitroFetchOptions<NitroFetchRequest> & Params<v1.paths[Path][Method]>)
		| (() => NitroFetchOptions<NitroFetchRequest> & Params<v1.paths[Path][Method]>);
}) => {
	const { path: parametersPaths } = (fetchOptions || {}) as unknown as { path: Record<string, string> };
	const headers = useRequestHeaders();
	const isQuasarClient = isClient && typeof window.__Q_META__ === "object";
	const baseURL = isQuasarClient ? undefined : "/api";
	const endpoint = replacePathParameters(path, parametersPaths);
	const asyncData = useAsync<Response, DataE, DataT, PickKeys, DefaultT>(
		options?.key || endpoint,
		() => {
			const requestParams = {
				baseURL,
				headers,
				...(typeof fetchOptions === "function" ? fetchOptions() : fetchOptions),
				method: method as PossibleMethods | undefined
			};

			const hashStr = hash({
				endpoint: options?.key || endpoint,
				requestParams
			});

			if (dedupePromises.has(hashStr)) {
				return dedupePromises.get(hashStr);
			}
			const promise = $fetch<Response>(endpoint, requestParams);

			dedupePromises.set(hashStr, promise);

			promise.finally(() => {
				dedupePromises.delete(hashStr);
			});

			return promise;
		},
		options
	);

	return asyncData;
};

export default useAsyncFetch;

So I think it would be cool to have it internally πŸ˜‡ Let me know please if you see some issues. That's my first PR in Nuxt. πŸ₯‡

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

Copy link

stackblitz bot commented Dec 1, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

nuxt-studio bot commented Dec 1, 2023

βœ… Live Preview ready!

Name Edit Preview Latest Commit
Nuxt Docs Edit on Studio β†—οΈŽ View Live Preview 3de69d8

@github-actions github-actions bot added the 3.x label Dec 1, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2023

Codecov Report

All modified and coverable lines are covered by tests βœ…

❗ No coverage uploaded for pull request base (main@4981e32). Click here to learn what that means.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #24564   +/-   ##
=======================================
  Coverage        ?   58.65%           
=======================================
  Files           ?        5           
  Lines           ?      861           
  Branches        ?       46           
=======================================
  Hits            ?      505           
  Misses          ?      356           
  Partials        ?        0           

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

Copy link
Member

@danielroe danielroe 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 a nice idea.

Note that it introduces a use of dedupe that is the opposite from the current usage (as an option to refresh). refresh({ dedupe: false }) means 'do not cancel existing requests in favour of this new one'.

But this dedupe: true means `do not make any new requests if there is an existing pending request.

I am not in fact sure that my original choice of dedupe as the option for refresh was the most semantically correct but I think the usage should align between option to useAsyncData and the option to refresh().

I would propose making dedupe accept these options:

  • 'cancel' (or true) - cancels existing requests when a new one is made <- default option
  • 'defer' (or false) - does not make new requests at all if there is a pending request

We would mark true or false types as deprecated and remove at a type level in a future minor.

Suggests welcome from @nuxt/core on naming choices here. πŸ™

@theguriev
Copy link
Contributor Author

This is a nice idea.

Note that it introduces a use of dedupe that is the opposite from the current usage (as an option to refresh). refresh({ dedupe: false }) means 'do not cancel existing requests in favour of this new one'.

But this dedupe: true means `do not make any new requests if there is an existing pending request.

I am not in fact sure that my original choice of dedupe as the option for refresh was the most semantically correct but I think the usage should align between option to useAsyncData and the option to refresh().

I would propose making dedupe accept these options:

  • 'cancel' (or true) - cancels existing requests when a new one is made <- default option
  • 'defer' (or false) - does not make new requests at all if there is a pending request

We would mark true or false types as deprecated and remove at a type level in a future minor.

Suggests welcome from @nuxt/core on naming choices here. πŸ™

Thank you! Yes it allows you to use the fetch anywhere without worrying about it being duplicated. Very useful in large companies when it's hard to keep track.

I updated my PM but not sure I understood you correctly. Please take a look when you get a chance. Thank you so much! ❀️

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

Would you add a test in this section (that is, the 'describe' block) of the runtime composables test? πŸ™

describe('useAsyncData', () => {
it('should work at basic level', async () => {
const res = useAsyncData(() => Promise.resolve('test'))
expect(Object.keys(res)).toMatchInlineSnapshot(`
[
"data",
"pending",
"error",
"status",
"execute",
"refresh",
]
`)
expect(res instanceof Promise).toBeTruthy()
expect(res.data.value).toBe(null)
await res
expect(res.data.value).toBe('test')
})

@danielroe danielroe requested a review from pi0 December 5, 2023 15:52
@theguriev
Copy link
Contributor Author

Would you add a test in this section (that is, the 'describe' block) of the runtime composables test? πŸ™

describe('useAsyncData', () => {
it('should work at basic level', async () => {
const res = useAsyncData(() => Promise.resolve('test'))
expect(Object.keys(res)).toMatchInlineSnapshot(`
[
"data",
"pending",
"error",
"status",
"execute",
"refresh",
]
`)
expect(res instanceof Promise).toBeTruthy()
expect(res.data.value).toBe(null)
await res
expect(res.data.value).toBe('test')
})

ofc. Here it is 8aa08d4

Let me know please if I can improve it somehow. Thank you! :)

@Jamie4224
Copy link
Contributor

Love this functionality! I implemented this by storing the useFetch in a pinia store, but for bigger projects with a lot of data sources, this is better!

@danielroe danielroe changed the title feat(nuxt): add dedupe option to the useAsyncData feat(nuxt): add dedupe option to useAsyncData and useFetch Dec 12, 2023
@danielroe danielroe added this to the 3.9 milestone Dec 12, 2023
@danielroe danielroe changed the title feat(nuxt): add dedupe option to useAsyncData and useFetch feat(nuxt): add dedupe option for data fetching composables Dec 14, 2023
@danielroe danielroe merged commit 8ccafb1 into nuxt:main Dec 14, 2023
37 checks passed
@github-actions github-actions bot mentioned this pull request Dec 14, 2023
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.

None yet

4 participants