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 type imports Parsable and ParsableFactory are causing build fail #4397

Closed
sher opened this issue Mar 25, 2024 · 3 comments · Fixed by #4398
Closed

Unused type imports Parsable and ParsableFactory are causing build fail #4397

sher opened this issue Mar 25, 2024 · 3 comments · Fixed by #4398
Labels
enhancement New feature or request help wanted Issue caused by core project dependency modules or library TypeScript Pull requests that update Javascript code
Milestone

Comments

@sher
Copy link
Contributor

sher commented Mar 25, 2024

Kiota generated request builder imports several types from abstractions package.

import { type BaseRequestBuilder, type Parsable, type ParsableFactory, type RequestConfiguration, type RequestInformation, type RequestsMetadata } from '@microsoft/kiota-abstractions';

Among them Parsable and ParsableFactory are never referenced in the generated code.
This is failing to build in projects where tsconfig.json has noUnusedLocals: true such as default Vitejs tsconfig.json.

@baywet baywet transferred this issue from microsoft/kiota-typescript Mar 25, 2024
@baywet
Copy link
Member

baywet commented Mar 25, 2024

Hi @sher
Thanks for using kiota and for reporting this.
Kiota already emits tslint and eslint disable "comments" so linter do not run on the generated code, reducing noise.
Now, because this is a TypeScript check (doesn't require any additional linter), it's still triggering.
I suggest we add an additional @ts-nocheck comment atop the files. documentation

The doc comments are being written here

writer.WriteLine("/* tslint:disable */");

And things are tested at the following places

Assert.Contains("/* tslint:disable */", result);

Assert.DoesNotContain("/* tslint:disable */", result);

Assert.Contains("/* eslint-disable */", result);

With all that information in mind, would you be willing to submit a pull request to address this?

@baywet baywet added enhancement New feature or request help wanted Issue caused by core project dependency modules or library TypeScript Pull requests that update Javascript code labels Mar 25, 2024
@baywet baywet added this to the Backlog milestone Mar 25, 2024
@baywet
Copy link
Member

baywet commented Mar 25, 2024

Actually, let me rephrase that suggestion, this is probably too big of a suppression because semantic checks will also be disbaled. This will make it much harder to catch any regression at build time.
Instead adding a // @ts-ignore directive before import statements would probably be safer

writer.WriteLine($"import {{ {codeUsing.Select(static x => GetAliasedSymbol(x.Symbol, x.Alias, x.ShouldUseTypeImport)).Distinct().OrderBy(static x => x).Aggregate(static (x, y) => x + ", " + y)} }} from '{codeUsing.Key}';");

@sher
Copy link
Contributor Author

sher commented Mar 25, 2024

Hi @baywet
Thank you for the feedback. Submitted a PR #4398 PTAL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Issue caused by core project dependency modules or library TypeScript Pull requests that update Javascript code
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants