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

Unused @ts-expect-error comment #589

Closed
DonnyVerduijn opened this issue May 17, 2024 · 17 comments
Closed

Unused @ts-expect-error comment #589

DonnyVerduijn opened this issue May 17, 2024 · 17 comments
Labels
bug 🔥 Something isn't working client Client package related question Further information is requested

Comments

@DonnyVerduijn
Copy link

Description

Given the following tsconfig.json configuration, typescript warns about an unused @ts-expect-error comment in core/request.ts at line 28 (i assume this file is input agnostic to that point but i'll share the contents.)

  "target": "ES2020",
  "useDefineForClassFields": true,
  "lib": ["ES2020", "DOM", "DOM.Iterable"],
  "module": "ESNext",
  "skipLibCheck": true,

  /* Bundler mode */
  "moduleResolution": "bundler",
  "allowImportingTsExtensions": true,
  "allowSyntheticDefaultImports": true,
  "exactOptionalPropertyTypes": true,
  "esModuleInterop": true,
  "resolveJsonModule": true,
  "isolatedModules": true,
  "noEmit": true,
  "jsx": "react-jsx",
  "jsxImportSource": "@emotion/react",

  /* Linting */
  "strict": true,

The contents of core/request.ts:

export const base64 = (str: string): string => {
  try {
    return btoa(str);
  } catch (err) {
    // @ts-expect-error
    return Buffer.from(str).toString('base64');
  }
};

OpenAPI specification (optional)

No response

Configuration

No response

System information (optional)

No response

@DonnyVerduijn DonnyVerduijn added the bug 🔥 Something isn't working label May 17, 2024
@mrlubos
Copy link
Contributor

mrlubos commented May 17, 2024

@DonnyVerduijn does this prevent you from generating a client?

@mrlubos mrlubos added the client Client package related label May 17, 2024
@DonnyVerduijn
Copy link
Author

DonnyVerduijn commented May 17, 2024

No. It's just an error in the editor. This doesn't fail tsc afaik.

@DonnyVerduijn
Copy link
Author

It does! Didn't know that.

src/__generated/api/core/request.ts:28:5 - error TS2578: Unused '@ts-expect-error' directive.

28     // @ts-expect-error
       ~~~~~~~~~~~~~~~~~~~

src/__generated/api/services.gen.ts:104:31 - error TS2379: Argument of type '{ method: "POST"; url: string; formData: { fileName?: Blob | File; } | undefined; mediaType: string; errors: { 500: string; }; }' is not assignable to parameter of type 'ApiRequestOptions' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the types of the target's properties.
  Types of property 'formData' are incompatible.
    Type '{ fileName?: Blob | File; } | undefined' is not assignable to type 'Record<string, unknown>'.
      Type 'undefined' is not assignable to type 'Record<string, unknown>'.

104     return __request(OpenAPI, {
                                  ~
105       method: 'POST',
    ~~~~~~~~~~~~~~~~~~~~~
... 
111       },
    ~~~~~~~~
112     });
    ~~~~~


Found 2 errors in 2 files.

Errors  Files
     1  src/__generated/api/core/request.ts:28
     1  src/__generated/api/services.gen.ts:104

@DonnyVerduijn
Copy link
Author

The problem with this, is that i'm generating the client postinstall (the client is not versioned), so during a CI run, tsc would break the pipeline.

@mrlubos
Copy link
Contributor

mrlubos commented May 24, 2024

Same question as the other issue, does using a standalone client solve this?

@mrlubos mrlubos added the question Further information is requested label May 24, 2024
@DonnyVerduijn
Copy link
Author

Same question as the other issue, does using a standalone client solve this?

Yes, although i prefer using the services. It has a lot of benefits in terms of intellisense, which is the primary reason to use this generator. What are you're thoughts on that, given the effort you're putting into this?

@DonnyVerduijn
Copy link
Author

By the way, i just had to revert @hey-api/openapi-ts back to 0.45.1 after upgrading. It seems there is a regression in 0.46.1

@mrlubos
Copy link
Contributor

mrlubos commented May 24, 2024

What's the regression @DonnyVerduijn? And to be clear, you are more than welcome to continue using services. The client itself doesn't have a good type support anyway since I expect you to use services. Have a look at the demo https://heyapi.vercel.app/openapi-ts/clients.html#fetch-api

@DonnyVerduijn
Copy link
Author

The referenced services were missing. I cleaned the working directory to be sure and reverted all changes (even removing fetch-client from node_modules), but 0.46.1 was not able to generate the services.

@mrlubos
Copy link
Contributor

mrlubos commented May 24, 2024

What does your config look like?

@DonnyVerduijn
Copy link
Author

import * as fs from 'fs';
import { createClient } from '@hey-api/openapi-ts';
import { load } from 'js-yaml';

export const generate = async (
  yamlFilePath: string = './openapi.yml',
  outputDir: string = './src/__generated/api'
) => {
  try {
    // Read YAML file and parse it
    const yamlContent = fs.readFileSync(yamlFilePath, 'utf8');
    const jsonSpec = load(yamlContent) as Record<string, unknown>;

    // Generate TypeScript client
    await createClient({
      client: 'fetch',
      // debug: true,
      input: Object.assign(jsonSpec, {
        servers: [{ url: '/api' }],
      }),
      output: {
        format: 'prettier',
        lint: 'eslint',
        path: outputDir,
      },
      schemas: {
        export: true,
        type: 'json',
      },
      services: {
        export: true,
        name: '{{name}}Service',
      },
      types: {
        dates: true,
        enums: 'typescript',
        export: true,
        name: 'preserve',
      },
    });

    console.log('Client generation completed successfully.');
  } catch (error) {
    console.error('Failed to generate client:', error);
  }
};

@DonnyVerduijn
Copy link
Author

What's the regression @DonnyVerduijn? And to be clear, you are more than welcome to continue using services. The client itself doesn't have a good type support anyway since I expect you to use services. Have a look at the demo https://heyapi.vercel.app/openapi-ts/clients.html#fetch-api

There might actually be a bit of a misunderstanding in terms of what we call services. In openapi-ts 0.45.1, we have a set of services where each service is a class, that exposes its methods to call certain api endpoints. When i said it makes more sense to take this approach in terms of a better DX through intellisense, what i mean't to say is that with large apis, it becomes very hard to get an intuition when there is no categorization. So, when there is just one file that exposes the methods directly as exported functions (such as in the fetch-client example), it would really become a burden to use.

@mrlubos
Copy link
Contributor

mrlubos commented May 24, 2024

Did you follow the migration guide for 0.46.0? Sounds like you want to keep generating classes but I don't see that in your config

@DonnyVerduijn
Copy link
Author

Did you follow the migration guide for 0.46.0? Sounds like you want to keep generating classes but I don't see that in your config

I see. I'll take a look at it tomorrow. You should really consider including that information in your releases and use semantic versioning/opt-in migration strategies.

@mrlubos
Copy link
Contributor

mrlubos commented May 24, 2024

Good shout. Do you know a good strategy for including migration notes in releases? I didn't look into configuring changesets further (there were no changelogs before btw).

The project follows semver too, see semver/semver#333

@DonnyVerduijn
Copy link
Author

Good shout. Do you know a good strategy for including migration notes in releases? I didn't look into configuring changesets further (there were no changelogs before btw).

The project follows semver too, see semver/semver#333

I'm not very experienced with authoring libraries on github, but i assume it depends on how the releases are prepared. I would have to look into it to say something specific.

In terms of following semver, the point is to make it easy to track down breaking changes through the release notes and allow pinning major versions if necessary.

Most of the time, breaking changes/migrations can be delayed without hindering adoption of new features. By introducing compatibility layers that are scrapped in the next major bump, the migrations are provided OOTB during minor bumps.

@DonnyVerduijn
Copy link
Author

This problem is resolved by using @hey-api/client-fetch. There are some other problems with @hey-api/client-fetch but closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🔥 Something isn't working client Client package related question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants
@DonnyVerduijn @mrlubos and others