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

fix: deepMerge type issue for some variables #286

Closed
wants to merge 2 commits into from

Conversation

sschw
Copy link
Contributor

@sschw sschw commented Feb 5, 2025

The current deepMerge function is mapping two objects of type T.

However the fetcherOptions of Context are general fetcherOptions while the variables and what is required by the fetch-function are request specific.

This causes type issues in the generated code. e.g. here in the example project:

  const { fetcherOptions } = useGithubContext();
  return reactQuery.useMutation<
    undefined,
    ActionsSetGithubActionsPermissionsOrganizationError,
    ActionsSetGithubActionsPermissionsOrganizationVariables
  >({
    mutationFn: (
      variables: ActionsSetGithubActionsPermissionsOrganizationVariables
    ) =>
      fetchActionsSetGithubActionsPermissionsOrganization(  // Expected type: ActionsSetGithubActionsPermissionsOrganizationVariables
        deepMerge(fetcherOptions, variables)                // Returned type: GithubContext["fetcherOptions"]
      ),
    ...options,
  });

Type definition:

export type ActionsSetGithubActionsPermissionsOrganizationVariables = {
  body: ActionsSetGithubActionsPermissionsOrganizationRequestBody;
  pathParams: ActionsSetGithubActionsPermissionsOrganizationPathParams;
} & GithubContext["fetcherOptions"];

According to the type definition, the variables are a type extending the fetcherOptions. This means, the deepMerge signature should be more like <T, U extends T>(target: T, source: U): U

I tweeked the existing function a bit to use above signature using a cast to the type we want to return. Also added || {} to prevent possible issues with nested calls when returnType might be undefined and returnType[key] call would therefore fail.
Function could also easily be changed to this signature <T, U>(target: T, source: U): T & U but left close to this use case.

I also updated the generated files in the example project.

@sschw sschw force-pushed the fix-deepmerge-signature branch from cb9298e to ac5afe6 Compare February 5, 2025 18:24
@fabien0102
Copy link
Owner

I will close this one for now, this tweak is not working with the skipToken support #289

image

(I’m not happy about the as any in my implementation, so if you find something better please open another PR 🙏)

@fabien0102 fabien0102 closed this Feb 8, 2025
@sschw
Copy link
Contributor Author

sschw commented Feb 10, 2025

It would be a little change to make it compatible, as the generics can be <T, S> instead of <T, S extends T>. By just removing extends T from the definition, it would accept the skipToken, too.

However, I don't think, this is the right solution as I think, deepMerge in its current implementation should not allow accepting the skipToken as it would not return the skipToken.
When merging source into target, it would just return target if source is the skipToken. (See Object.assign({ header: {} }, Symbol("Test")) )

I'd instead suggest, to never call deepMerge with the skipToken and already do a check in the components functions.

See updated PR.

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.

2 participants