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(compute-key): add support for mapKeyToCacheKey #71

Merged
merged 10 commits into from
Mar 6, 2023

Conversation

code-forger
Copy link
Member

this option allows consumers to directly vary their cache key

Motivation and Context

This change enables more advanced caching features, where the cached data is not directly tied to a single complete url.

How Has This Been Tested?

unit tests, run locally in a module.
This is a non-breaking change. No existing unit test was changed of fails.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (adding or updating documentation)
  • Dependency update

Checklist:

  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • My changes are in sync with the code style of this project.
  • There aren't any other open Pull Requests for the same issue/update.
  • These changes should be applied to a maintenance branch.
  • I have added the Apache 2.0 license header to any new files created.

What is the Impact to Developers Using Fetchye?

New functionality

this option allows consumers to directly vary their cache key
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 28, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a693a07:

Sandbox Source
quick-install Configuration
fetchye-provider-install Configuration
fetchye-redux-provider-install Configuration
nextjs-fetchye-ssr Configuration

Co-authored-by: Matthew Mallimo <matthew.c.mallimo@aexp.com>
@Matthew-Mallimo Matthew-Mallimo requested a review from a team February 28, 2023 12:14
Copy link
Member

@10xLaCroixDrinker 10xLaCroixDrinker left a comment

Choose a reason for hiding this comment

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

Can you include a test that does a transform on the key instead of just replacing it?

packages/fetchye/__tests__/computeKey.spec.js Outdated Show resolved Hide resolved
packages/fetchye/__tests__/computeKey.spec.js Outdated Show resolved Hide resolved
packages/fetchye/__tests__/computeKey.spec.js Outdated Show resolved Hide resolved
packages/fetchye/src/computeKey.js Show resolved Hide resolved
packages/fetchye/__tests__/computeKey.spec.js Outdated Show resolved Hide resolved
packages/fetchye/__tests__/computeKey.spec.js Outdated Show resolved Hide resolved
@@ -58,4 +67,55 @@ describe('computeKey', () => {
};
expect(computeKey('uri', firstOptions).hash).toBe(computeKey('uri', secondOptions).hash);
});

it('should return a different, stable hash, if the option mapKeyToCacheKey is passed', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This test cannot assert that the key is different as the spec states since it only generates one key. There is nothing to compare it to

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in a693a07

expect(computeHash).toHaveBeenCalledWith(['ABCD-optionKeyValue', { optionKeyMock: 'optionKeyValue' }], { respectType: false });
});

it('should return the same key if the option mapKeyToCacheKey returns the same string as the key', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This test has the same issue as the one on line 71

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in a693a07

@10xLaCroixDrinker 10xLaCroixDrinker merged commit 1f29629 into main Mar 6, 2023
@10xLaCroixDrinker 10xLaCroixDrinker deleted the feat/mapKeyToCacheKey branch March 6, 2023 17:55
@aneudy1702
Copy link

@code-forger , thanks this is very promising. how does this work with the SSR request?

namely,

const fetchye = makeOneServerFetchye({ store, fetchClient });

is makeOneServerFetchye able to take this additional parameter to map the cache key?

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

5 participants