Skip to content

Bug fix(accessControlProvider): resolve cacheTime issue for can function (#6749) #6753

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

O-BERNARDOFOEGBU
Copy link

@O-BERNARDOFOEGBU O-BERNARDOFOEGBU commented Apr 15, 2025

…6749

PR Checklist

Please check if your PR fulfills the following requirements:

Bugs / Features

What is the current behavior?

The can function was being executed multiple times for each resource upon page reloads or when wrapped with CanAccess, even when the result could be cached.

cacheTime and staleTime configurations were not being respected, leading to performance degradation.

What is the new behavior?

Fixes Issue #6749: cacheTime and staleTime are now respected, ensuring that the results of the can function are cached for the specified time.

The can function will no longer run multiple times unnecessarily when resources are already evaluated, improving the page load time and performance.

Notes for reviewers

Please ensure that the cache handling logic is thoroughly tested, and the behavior is consistent across different resources.

If you have any concerns regarding the caching behavior, let me know, and we can refine it further.

@O-BERNARDOFOEGBU O-BERNARDOFOEGBU requested a review from a team as a code owner April 15, 2025 19:51
Copy link

changeset-bot bot commented Apr 15, 2025

⚠️ No Changeset found

Latest commit: b6f8c34

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

Copy link

netlify bot commented Apr 15, 2025

Deploy Preview for refine-doc-live-previews ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b6f8c34
🔍 Latest deploy log https://app.netlify.com/sites/refine-doc-live-previews/deploys/6803745ef4ef980008624879
😎 Deploy Preview https://deploy-preview-6753--refine-doc-live-previews.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.


// import { getXRay } from "@refinedev/devtools-internal";
Copy link
Member

Choose a reason for hiding this comment

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

I think these commented lines were added by mistake.

Copy link
Author

Choose a reason for hiding this comment

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

yes.... skipped my mind to take 'em off. will do that in a few minutes.....

Copy link
Author

Choose a reason for hiding this comment

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

@alicanerdurmaz, the commented lines has been removed.

Comment on lines +71 to +77
// Check if we have a cached value and if it's still valid
if (cacheEntry && Date.now() - cacheEntry.timestamp < cacheDuration) {
return { data: cacheEntry.result } as UseQueryResult<CanReturnType>;
}

const queryResponse = useQuery<CanReturnType>({
...mergedQueryOptions,
Copy link
Member

Choose a reason for hiding this comment

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

you must call hooks unconditionally https://react.dev/reference/rules/rules-of-hooks or it will

Thank you for helping to develop Refine, but we’d appreciate it if you could open the PR only after you’ve tested it. If you try running the access-control-casbin example you’ll see this error and the project won’t work:

Uncaught Error: Rendered fewer hooks than expected. This may be caused by an accidental early return statement.
    at renderWithHooks (chunk-WBKLWWXU.js?v=52d2936b:11623:19)
    at updateFunctionComponent (chunk-WBKLWWXU.js?v=52d2936b:14610:28)
    at beginWork (chunk-WBKLWWXU.js?v=52d2936b:15952:22)
    at beginWork$1 (chunk-WBKLWWXU.js?v=52d2936b:19789:22)
    at performUnitOfWork (chunk-WBKLWWXU.js?v=52d2936b:19234:20)
    at workLoopSync (chunk-WBKLWWXU.js?v=52d2936b:19173:13)
    at renderRootSync (chunk-WBKLWWXU.js?v=52d2936b:19152:15)
    at recoverFromConcurrentError (chunk-WBKLWWXU.js?v=52d2936b:18772:28)
    at performSyncWorkOnRoot (chunk-WBKLWWXU.js?v=52d2936b:18915:28)
    at flushSyncCallbacks (chunk-WBKLWWXU.js?v=52d2936b:9143:30)

* @see {@link https://refine.dev/docs/api-reference/core/hooks/accessControl/useCan} for more details.
* Custom cache for `can` function results to optimize performance.
*/
const canCache = new Map<string, { result: CanReturnType; timestamp: number }>();
Copy link
Member

Choose a reason for hiding this comment

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

I don’t think we need a custom cache here. TanStack Query already has an excellent caching mechanism.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] accessControlProvider cacheTime does not seem to be working
2 participants