fix: avoid duplicate variable name in swr function

[soartec-lab]

Status

READY

Description

fix #800

When using query for an API parameter defined in OpenAPI, the name of the argument in the automatically generated function and the name of the argument within the function collide.
In that case, we changed the variable name query to _query.
Variables other than query are not considered because they are rarely used as parameters.

Similar problems occur with client other than swr, so we will deal with them in order once this PR is merged.

Related PRs

none

Todos

• Tests
• Documentation
• Changelog Entry (unreleased)

Steps to Test or Reproduce

1. Specify query in the parameter name as below:

openapi: '3.0.0'
info:
  version: 1.0.0
  title: Swagger
  license:
    name: MIT
paths:
  /pets/{query}:
    get:
      summary: Info for a specific pet
      operationId: showPetById
      tags:
        - pets
      parameters:
        - name: query
          in: path
          required: true
          description: The id of the pet to retrieve
          schema:
            type: string
      responses:
        '200':
          description: Expected response to a valid request
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Pet'
        default:
          description: unexpected error
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Error'
components:
  schemas:
    Pet:
      type: object
      required:
        - id
        - name
      properties:
        id:
          type: integer
          format: int64
        name:
          type: string
        tag:
          type: string
    Error:
      type: object
      required:
        - code
        - message
      properties:
        code:
          type: integer
          format: int32
        message:
          type: string

2. Specify swr for client in orval.config.js as bellow:

module.exports = {
  'petstore-file': {
    input: {
      target: './petstore.yaml',
    },
    output: {
      mode: 'tags-split',
      client: 'swr',
      target: 'src/gen/endpoints',
      schemas: 'src/gen/model',
      indexFiles: false,
    },
  },
};

3. orval execute

orval

4. The function is output where the name of the variable is replaced from query to _query

export const useShowPetById = <TError = AxiosError<Error>>(
  query: string,
  options?: {
    swr?: SWRConfiguration<Awaited<ReturnType<typeof showPetById>>, TError> & {
      swrKey?: Key;
      enabled?: boolean;
    };
    axios?: AxiosRequestConfig;
  },
) => {
  const { swr: swrOptions, axios: axiosOptions } = options ?? {};

  const isEnabled = swrOptions?.enabled !== false && !!query;
  const swrKey =
    swrOptions?.swrKey ?? (() => (isEnabled ? getShowPetByIdKey(query) : null));
  const swrFn = () => showPetById(query, axiosOptions);

  const _query = useSwr<Awaited<ReturnType<typeof swrFn>>, TError>(
    swrKey,
    swrFn,
    swrOptions,
  );

  return {
    swrKey,
    ..._query,
  };
};

[melloware]

OK then rename hasParamNamedQuery to hasParamReservedWord so people know why you are looking specifically for the word query they will know its a reserved word.

[soartec-lab]

@melloware

Please allow me to add to the explanation.

The purpose of using hasParamNameQuery in the variable name is not whether it is a reserved word or not, but simply to check whether query is used as a parameter.

Let's consider a case where another reserved word has increased.
For example, suppose you want to treat swrFn as a reserved word and add _ to it.
In that case, I think the following will be added:

const hasParamNameSwrFn = props.some(
  (param: GetterProp) => param.name === 'swrFn',
);
const queryResultVarName = hasParamNameSwrFn ? '_swrFn' : 'swrFn';

Because, variables used within functions with the same name are marked with _ depending on the existence of individual specified parameters, not whether the parameters contain reserved words. However, please note that this is just an example and there are other options if you actually want to respond.

From this, I think that the original hasParamNameQuery represents the original intention of the variable name rather than hasParamReservedWord as commented.

And I thought that the reason was expressed in the code, so it was sufficient, but what do you think?

[melloware]

I disagree the word query is to generic in terms of React Query, Vue Query, SWR? So this code tells me no reason wht you are looking for hasParamNamedQuery its too specific with no reasoning why you are doing is.

"looking for a reserved word, preventing its use by appending an underscore so it does not collide with other usages" From looking at your code I can't tell any of that and no where does it mention query is a reserved word you are looking for.

So someone looking at this code in 1 year will have the spend the time to think "why are they doing this" where as naming things properly like "checkReservedWord" or "cleanReservedWords" tells the developer exactly what you are doing without thinking about it.

[melloware]

At the very least put a comment above the line.

// #800 Convert param named query to _query to prevent collision

[soartec-lab]

@melloware
Got it. I respect your opinion and change the function name from "hasParamNamedQuery" to "hasParamReservedWord".

[soartec-lab]

Fixed as bellow:

https://github.com/anymaniax/orval/compare/5693ebe1387492d5c25e8bc0c512fd4e8db63148..146f9bb90da54fbb012f2948c65280a445ca70bf

[melloware]

See comment