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

Add Missing Else to enhanceEndpoints Function #2386

Merged
merged 4 commits into from
Jun 10, 2022

Conversation

kinson
Copy link
Contributor

@kinson kinson commented Jun 5, 2022

Overview

Addresses #2334 .

This pr adds a missing else to the enhanceEndpoints function to improve the clarity of the function. It also adds an example of using it to the documentation.

Regarding the original issue:

This needs to be a else or the Object.assign will assign the function to the endpoint.

Based on my understanding of Object.assign, passing a function as an assign will return a copy of the original object with no modifications. I created a codesandbox to demonstrate it (forgive me if there is a better template to use for such a simple example). As a result, the function won't affect the endpoint as the code currently is written. Let me know if I am misunderstanding the issue though, or if my premise is incorrect.

Here is the code snippet again for reference:

for (const [endpointName, partialDefinition] of Object.entries(
endpoints
)) {
if (typeof partialDefinition === 'function') {
partialDefinition(context.endpointDefinitions[endpointName])
}
Object.assign(
context.endpointDefinitions[endpointName] || {},
partialDefinition
)
}

Even if the lack of an else will not cause a bug, it still does not make much sense to call Object.assign when a function is passed, because it does not do anything. So, I still added the else in this pr for clarity and readability.

The issue asked for tests as well, however when I went to adds tests I realized I was mostly duplicating a test that was already in the test suite:

test('modify', () => {
api.enhanceEndpoints({
endpoints: {
query1: {
query: (x) => {
expectExactType('in1' as const)(x)
return 'modified1'
},
},
query2(definition) {
definition.query = (x) => {
expectExactType('in2' as const)(x)
return 'modified2'
}
},
mutation1: {
query: (x) => {
expectExactType('in1' as const)(x)
return 'modified1'
},
},
mutation2(definition) {
definition.query = (x) => {
expectExactType('in2' as const)(x)
return 'modified2'
}
},
// @ts-expect-error
nonExisting: {},
},
})
storeRef.store.dispatch(api.endpoints.query1.initiate('in1'))
storeRef.store.dispatch(api.endpoints.query2.initiate('in2'))
storeRef.store.dispatch(api.endpoints.mutation1.initiate('in1'))
storeRef.store.dispatch(api.endpoints.mutation2.initiate('in2'))
expect(baseQuery.mock.calls).toEqual([
['modified1', commonBaseQueryApi, undefined],
['modified2', commonBaseQueryApi, undefined],
['modified1', { ...commonBaseQueryApi, forced: undefined }, undefined],
['modified2', { ...commonBaseQueryApi, forced: undefined }, undefined],
])
})
})

I am happy to add more tests πŸ€“ , but I wondering what test cases may remain for this. I removed my test change from this pr because I didn't want to add more weight to the codebase without adding any testing value.

Lastly, is there a way for me to preview the docs changes in the format they are rendered? I wanted to provide a before/after screenshot but I am probably missing a very obvious set of steps to get it up and running! CI did it for me 😍 (and I figured out how to do it locally as well)

Thanks for creating this package, and taking the time to read this wall of text ✌🏻 πŸ˜„

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 5, 2022

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 2e71d34:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration
rsk-github-issues-example Configuration
reduxjs/redux-toolkit Configuration
@examples-query-react/advanced Configuration
reduxjs/redux-toolkit Configuration
crazy-clarke-hluqv3 PR

For example, `enhanceEndpoints` can be used to modify caching behaviour by changing the values of `providesTags`, `invalidatesTags`, and `keepUnusedDataFor`:

```ts
const enhancedApi = api.enchanceEndpoints({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like CI may have failed due to this line:
https://app.netlify.com/sites/redux-starter-kit-docs/deploys/629d1fee7ab8150008c71b33#L301

I will take a closer look and fix this!

@kinson kinson force-pushed the add-else-to-enhance-endpoints branch from 8e94b5a to 787a30f Compare June 6, 2022 01:45
@netlify
Copy link

netlify bot commented Jun 6, 2022

βœ… Deploy Preview for redux-starter-kit-docs ready!

Name Link
πŸ”¨ Latest commit 2e71d34
πŸ” Latest deploy log https://app.netlify.com/sites/redux-starter-kit-docs/deploys/62a31ef64ddd76000972edca
😎 Deploy Preview https://deploy-preview-2386--redux-starter-kit-docs.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 settings.

@phryneas
Copy link
Member

Thank you for looking deeper into this!

I think the test is enough - to be honest, I just forgot about the existence of that test :)

I've tweaked the example a bit so it works without the : any and am going to get this in - it makes the code base a bit more sane and improves the docs πŸŽ‰

@phryneas phryneas merged commit 9cf6cc0 into reduxjs:master Jun 10, 2022
@kinson
Copy link
Contributor Author

kinson commented Jun 10, 2022

Thanks for the updates and finding a way to remove the pesky any!

@kinson kinson deleted the add-else-to-enhance-endpoints branch June 10, 2022 13:49
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.

None yet

2 participants