fix(clerk-js): Fetching custom role in OrganizationSwitcher with cache by panteliselef · Pull Request #2430 · clerk/javascript

panteliselef · 2023-12-21T10:58:56Z

Description

Problem

In order to display a custom role we need to fetch them, so far we didn't have a robust way of fetching resources in clerk-js. A while back i had implemented useFetch, today I'm extending it in order to use a cache in order to fetch and share resources across components more efficiently.

Before

When a component need to display the custom roles we had to fetch those each time the component was mounted.

After

We fetch the resource once, other subscribers will not trigger a refetch unless the data is stale (in cache for 2 mins).

Goal

The goal of this PR is to provide a good enough fix for the issue with an efficient solution. This is a bug in v4 as well, and a simple solution is better when backporting is necessary. If we think this solution is limiting, we should consider migrating to swr as its bundlesize is minimal and much more robust.

For tests:

We need to clear cache before each test

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

Packages affected

In order to do this efficiently and to avoid requesting everytime the switcher opens, a simple cache inside useFetch was implemented.

changeset-bot · 2023-12-21T10:58:59Z

🦋 Changeset detected

Latest commit: 67c450c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@clerk/clerk-js	Patch
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

brkalow

We need to tweak the logic around params in useCache to ensure we're dealing with a stable cache key.

brkalow · 2023-12-21T16:53:13Z

- const requestStatus = useLoadingStatus({
- status: 'loading',
- });
+ const cache = useCache(params);


It appears that params is regularly an object here that is not memoized. If I'm understanding the useCache implementation correctly, we're using the raw params value as a cache key. This means that every re-render could potentially introduce a new entry in the cache. We likely need to stringify the object in a deterministic way (e.g. sorted) to get a consistent cache key (I see we're doing it below).
Additionally, the useCallback usage within useCache is not so useful for the same reason (params changing reference on each call).

Thanks for caching this @brkalow, shall i go the stringify approach ?

brkalow

💯

#2430)

#2430) (#2450)

panteliselef · 2023-12-21T10:58:56Z

Description

Problem

In order to display a custom role we need to fetch them, so far we didn't have a robust way of fetching resources in clerk-js. A while back i had implemented useFetch, today I'm extending it in order to use a cache in order to fetch and share resources across components more efficiently.

Before

When a component need to display the custom roles we had to fetch those each time the component was mounted.

After

We fetch the resource once, other subscribers will not trigger a refetch unless the data is stale (in cache for 2 mins).

Goal

The goal of this PR is to provide a good enough fix for the issue with an efficient solution. This is a bug in v4 as well, and a simple solution is better when backporting is necessary. If we think this solution is limiting, we should consider migrating to swr as its bundlesize is minimal and much more robust.

For tests:

We need to clear cache before each test

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

Packages affected

In order to do this efficiently and to avoid requesting everytime the switcher opens, a simple cache inside useFetch was implemented.

changeset-bot · 2023-12-21T10:58:59Z

🦋 Changeset detected

Latest commit: 67c450c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@clerk/clerk-js	Patch
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

brkalow

We need to tweak the logic around params in useCache to ensure we're dealing with a stable cache key.

brkalow · 2023-12-21T16:53:13Z

- const requestStatus = useLoadingStatus({
- status: 'loading',
- });
+ const cache = useCache(params);


It appears that params is regularly an object here that is not memoized. If I'm understanding the useCache implementation correctly, we're using the raw params value as a cache key. This means that every re-render could potentially introduce a new entry in the cache. We likely need to stringify the object in a deterministic way (e.g. sorted) to get a consistent cache key (I see we're doing it below).
Additionally, the useCallback usage within useCache is not so useful for the same reason (params changing reference on each call).

Thanks for caching this @brkalow, shall i go the stringify approach ?

brkalow

💯

#2430)

#2430) (#2450)

panteliselef · 2023-12-21T10:58:56Z

Description

Problem

In order to display a custom role we need to fetch them, so far we didn't have a robust way of fetching resources in clerk-js. A while back i had implemented useFetch, today I'm extending it in order to use a cache in order to fetch and share resources across components more efficiently.

Before

When a component need to display the custom roles we had to fetch those each time the component was mounted.

After

We fetch the resource once, other subscribers will not trigger a refetch unless the data is stale (in cache for 2 mins).

Goal

The goal of this PR is to provide a good enough fix for the issue with an efficient solution. This is a bug in v4 as well, and a simple solution is better when backporting is necessary. If we think this solution is limiting, we should consider migrating to swr as its bundlesize is minimal and much more robust.

For tests:

We need to clear cache before each test

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

Packages affected

In order to do this efficiently and to avoid requesting everytime the switcher opens, a simple cache inside useFetch was implemented.

changeset-bot · 2023-12-21T10:58:59Z

🦋 Changeset detected

Latest commit: 67c450c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@clerk/clerk-js	Patch
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

brkalow

We need to tweak the logic around params in useCache to ensure we're dealing with a stable cache key.

brkalow · 2023-12-21T16:53:13Z

- const requestStatus = useLoadingStatus({
- status: 'loading',
- });
+ const cache = useCache(params);


It appears that params is regularly an object here that is not memoized. If I'm understanding the useCache implementation correctly, we're using the raw params value as a cache key. This means that every re-render could potentially introduce a new entry in the cache. We likely need to stringify the object in a deterministic way (e.g. sorted) to get a consistent cache key (I see we're doing it below).
Additionally, the useCallback usage within useCache is not so useful for the same reason (params changing reference on each call).

Thanks for caching this @brkalow, shall i go the stringify approach ?

brkalow

💯

#2430)

#2430) (#2450)

panteliselef · 2023-12-21T10:58:56Z

Description

Problem

In order to display a custom role we need to fetch them, so far we didn't have a robust way of fetching resources in clerk-js. A while back i had implemented useFetch, today I'm extending it in order to use a cache in order to fetch and share resources across components more efficiently.

Before

When a component need to display the custom roles we had to fetch those each time the component was mounted.

After

We fetch the resource once, other subscribers will not trigger a refetch unless the data is stale (in cache for 2 mins).

Goal

The goal of this PR is to provide a good enough fix for the issue with an efficient solution. This is a bug in v4 as well, and a simple solution is better when backporting is necessary. If we think this solution is limiting, we should consider migrating to swr as its bundlesize is minimal and much more robust.

For tests:

We need to clear cache before each test

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

Packages affected

In order to do this efficiently and to avoid requesting everytime the switcher opens, a simple cache inside useFetch was implemented.

changeset-bot · 2023-12-21T10:58:59Z

🦋 Changeset detected

Latest commit: 67c450c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@clerk/clerk-js	Patch
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

brkalow

We need to tweak the logic around params in useCache to ensure we're dealing with a stable cache key.

brkalow · 2023-12-21T16:53:13Z

- const requestStatus = useLoadingStatus({
- status: 'loading',
- });
+ const cache = useCache(params);


It appears that params is regularly an object here that is not memoized. If I'm understanding the useCache implementation correctly, we're using the raw params value as a cache key. This means that every re-render could potentially introduce a new entry in the cache. We likely need to stringify the object in a deterministic way (e.g. sorted) to get a consistent cache key (I see we're doing it below).
Additionally, the useCallback usage within useCache is not so useful for the same reason (params changing reference on each call).

Thanks for caching this @brkalow, shall i go the stringify approach ?

brkalow

💯

#2430)

#2430) (#2450)

panteliselef · 2023-12-21T10:58:56Z

Description

Problem

In order to display a custom role we need to fetch them, so far we didn't have a robust way of fetching resources in clerk-js. A while back i had implemented useFetch, today I'm extending it in order to use a cache in order to fetch and share resources across components more efficiently.

Before

When a component need to display the custom roles we had to fetch those each time the component was mounted.

After

We fetch the resource once, other subscribers will not trigger a refetch unless the data is stale (in cache for 2 mins).

Goal

The goal of this PR is to provide a good enough fix for the issue with an efficient solution. This is a bug in v4 as well, and a simple solution is better when backporting is necessary. If we think this solution is limiting, we should consider migrating to swr as its bundlesize is minimal and much more robust.

For tests:

We need to clear cache before each test

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

Packages affected

In order to do this efficiently and to avoid requesting everytime the switcher opens, a simple cache inside useFetch was implemented.

changeset-bot · 2023-12-21T10:58:59Z

🦋 Changeset detected

Latest commit: 67c450c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@clerk/clerk-js	Patch
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

brkalow

We need to tweak the logic around params in useCache to ensure we're dealing with a stable cache key.

brkalow · 2023-12-21T16:53:13Z

- const requestStatus = useLoadingStatus({
- status: 'loading',
- });
+ const cache = useCache(params);


It appears that params is regularly an object here that is not memoized. If I'm understanding the useCache implementation correctly, we're using the raw params value as a cache key. This means that every re-render could potentially introduce a new entry in the cache. We likely need to stringify the object in a deterministic way (e.g. sorted) to get a consistent cache key (I see we're doing it below).
Additionally, the useCallback usage within useCache is not so useful for the same reason (params changing reference on each call).

Thanks for caching this @brkalow, shall i go the stringify approach ?

brkalow

💯

#2430)

#2430) (#2450)

panteliselef · 2023-12-21T10:58:56Z

Description

Problem

In order to display a custom role we need to fetch them, so far we didn't have a robust way of fetching resources in clerk-js. A while back i had implemented useFetch, today I'm extending it in order to use a cache in order to fetch and share resources across components more efficiently.

Before

When a component need to display the custom roles we had to fetch those each time the component was mounted.

After

We fetch the resource once, other subscribers will not trigger a refetch unless the data is stale (in cache for 2 mins).

Goal

The goal of this PR is to provide a good enough fix for the issue with an efficient solution. This is a bug in v4 as well, and a simple solution is better when backporting is necessary. If we think this solution is limiting, we should consider migrating to swr as its bundlesize is minimal and much more robust.

For tests:

We need to clear cache before each test

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

Packages affected

In order to do this efficiently and to avoid requesting everytime the switcher opens, a simple cache inside useFetch was implemented.

changeset-bot · 2023-12-21T10:58:59Z

🦋 Changeset detected

Latest commit: 67c450c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@clerk/clerk-js	Patch
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

brkalow

We need to tweak the logic around params in useCache to ensure we're dealing with a stable cache key.

brkalow · 2023-12-21T16:53:13Z

- const requestStatus = useLoadingStatus({
- status: 'loading',
- });
+ const cache = useCache(params);


It appears that params is regularly an object here that is not memoized. If I'm understanding the useCache implementation correctly, we're using the raw params value as a cache key. This means that every re-render could potentially introduce a new entry in the cache. We likely need to stringify the object in a deterministic way (e.g. sorted) to get a consistent cache key (I see we're doing it below).
Additionally, the useCallback usage within useCache is not so useful for the same reason (params changing reference on each call).

Thanks for caching this @brkalow, shall i go the stringify approach ?

brkalow

💯

#2430)

#2430) (#2450)

panteliselef · 2023-12-21T10:58:56Z

Description

Problem

In order to display a custom role we need to fetch them, so far we didn't have a robust way of fetching resources in clerk-js. A while back i had implemented useFetch, today I'm extending it in order to use a cache in order to fetch and share resources across components more efficiently.

Before

When a component need to display the custom roles we had to fetch those each time the component was mounted.

After

We fetch the resource once, other subscribers will not trigger a refetch unless the data is stale (in cache for 2 mins).

Goal

The goal of this PR is to provide a good enough fix for the issue with an efficient solution. This is a bug in v4 as well, and a simple solution is better when backporting is necessary. If we think this solution is limiting, we should consider migrating to swr as its bundlesize is minimal and much more robust.

For tests:

We need to clear cache before each test

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

Packages affected

In order to do this efficiently and to avoid requesting everytime the switcher opens, a simple cache inside useFetch was implemented.

changeset-bot · 2023-12-21T10:58:59Z

🦋 Changeset detected

Latest commit: 67c450c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@clerk/clerk-js	Patch
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

brkalow

We need to tweak the logic around params in useCache to ensure we're dealing with a stable cache key.

brkalow · 2023-12-21T16:53:13Z

- const requestStatus = useLoadingStatus({
- status: 'loading',
- });
+ const cache = useCache(params);


It appears that params is regularly an object here that is not memoized. If I'm understanding the useCache implementation correctly, we're using the raw params value as a cache key. This means that every re-render could potentially introduce a new entry in the cache. We likely need to stringify the object in a deterministic way (e.g. sorted) to get a consistent cache key (I see we're doing it below).
Additionally, the useCallback usage within useCache is not so useful for the same reason (params changing reference on each call).

Thanks for caching this @brkalow, shall i go the stringify approach ?

brkalow

💯

#2430)

#2430) (#2450)

panteliselef · 2023-12-21T10:58:56Z

Description

Problem

In order to display a custom role we need to fetch them, so far we didn't have a robust way of fetching resources in clerk-js. A while back i had implemented useFetch, today I'm extending it in order to use a cache in order to fetch and share resources across components more efficiently.

Before

When a component need to display the custom roles we had to fetch those each time the component was mounted.

After

We fetch the resource once, other subscribers will not trigger a refetch unless the data is stale (in cache for 2 mins).

Goal

The goal of this PR is to provide a good enough fix for the issue with an efficient solution. This is a bug in v4 as well, and a simple solution is better when backporting is necessary. If we think this solution is limiting, we should consider migrating to swr as its bundlesize is minimal and much more robust.

For tests:

We need to clear cache before each test

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

Packages affected

In order to do this efficiently and to avoid requesting everytime the switcher opens, a simple cache inside useFetch was implemented.

changeset-bot · 2023-12-21T10:58:59Z

🦋 Changeset detected

Latest commit: 67c450c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@clerk/clerk-js	Patch
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

brkalow

We need to tweak the logic around params in useCache to ensure we're dealing with a stable cache key.

brkalow · 2023-12-21T16:53:13Z

- const requestStatus = useLoadingStatus({
- status: 'loading',
- });
+ const cache = useCache(params);


It appears that params is regularly an object here that is not memoized. If I'm understanding the useCache implementation correctly, we're using the raw params value as a cache key. This means that every re-render could potentially introduce a new entry in the cache. We likely need to stringify the object in a deterministic way (e.g. sorted) to get a consistent cache key (I see we're doing it below).
Additionally, the useCallback usage within useCache is not so useful for the same reason (params changing reference on each call).

Thanks for caching this @brkalow, shall i go the stringify approach ?

brkalow

💯

#2430)

#2430) (#2450)

panteliselef added 2 commits December 21, 2023 12:18

fix(clerk-js): Fetching custom role in OrganizationSwitcher
2d10b3a
In order to do this efficiently and to avoid requesting everytime the switcher opens, a simple cache inside useFetch was implemented.

chore(clerk-js): Clear cache before each test
45f4999

panteliselef self-assigned this Dec 21, 2023

clerk-cookie added the clerk-js label Dec 21, 2023

panteliselef changed the title ~~fix(clerk-js): Fetching custom role in OrganizationSwitcher~~ fix(clerk-js): Fetching custom role in OrganizationSwitcher with cacheDec 21, 2023

panteliselef requested review from a team and tmilewski and removed request for a team and tmilewski December 21, 2023 10:59

panteliselef added 2 commits December 21, 2023 13:01

chore(clerk-js): Add changeset
6559ce8

chore(clerk-js): Clean up
033fba3

panteliselef force-pushed the elef/core-1242-custom-role-missing-label branch from 2724193 to 033fba3Compare December 21, 2023 11:25

brkalow requested changes Dec 21, 2023
View reviewed changes

panteliselef added 3 commits December 22, 2023 21:56

fix(clerk-js): Change WeakMap to Map and store a serialized key
f315922

chore(clerk-js): Add isValidating status
8132f4a

chore(clerk-js): Use useSyncExternalStore
67c450c

panteliselef requested a review from brkalow December 22, 2023 20:12

brkalow approved these changes Dec 22, 2023
View reviewed changes

panteliselef added this pull request to the merge queue Dec 22, 2023

github-merge-queueBot removed this pull request from the merge queue due to failed status checks Dec 22, 2023

panteliselef added this pull request to the merge queue Dec 22, 2023

Merged via the queue into main with commit 9040549Dec 22, 2023

panteliselef deleted the elef/core-1242-custom-role-missing-label branch December 22, 2023 22:05

clerk-cookie mentioned this pull request Dec 22, 2023
Version Packages (alpha-v5) #2449
Merged

panteliselef added a commit that referenced this pull request Dec 22, 2023
fix(clerk-js): Fetching custom role in OrganizationSwitcher with cache (
741546b
#2430)

panteliselef mentioned this pull request Dec 22, 2023
fix(clerk-js): Fetching custom role in OrganizationSwitcher with cache (#2430) #2450
Merged
24 tasks

panteliselef added a commit that referenced this pull request Dec 22, 2023
fix(clerk-js): Fetching custom role in OrganizationSwitcher with cache (
d2e5485
#2430)

github-merge-queueBot pushed a commit that referenced this pull request Jan 2, 2024
fix(clerk-js): Fetching custom role in OrganizationSwitcher with cache (
04b6b18
#2430) (#2450)

LekoArts mentioned this pull request Mar 13, 2024
fix(themes): Add tslib and @clerk/types as dependencies to @clerk/themes (#2665) #2987
Merged

LekoArts mentioned this pull request Aug 2, 2024
chore(repo): Correct changeset #3883
Merged