Skip to content

[js-api-parser] Fix constructor rendering for destructured parameters#14451

Open
Copilot wants to merge 30 commits intomainfrom
copilot/fix-sanitzer-constructor-rendering
Open

[js-api-parser] Fix constructor rendering for destructured parameters#14451
Copilot wants to merge 30 commits intomainfrom
copilot/fix-sanitzer-constructor-rendering

Conversation

Copy link
Contributor

Copilot AI commented Mar 10, 2026

  • Fix constructor.ts to skip destructured parameters (names starting with {)
  • Add test in constructor.spec.ts for the destructured parameter case
  • Resolve merge conflicts - synced all js-api-parser files with main (TokenCollector refactoring from PR Remove fallback of the old token parser generator #14376)
  • Rebased onto latest main (minimatch bump, sdk-testgen ESLint migration, azsdk-cli codeowners, dependency bumps)
  • Improve destructured parameter test: set excerptText to the real API Extractor constructor signature so getParameterPropertyModifiers exercises the TypeScript AST parsing path, and add precise assertions (no stray commas/colons)
  • All 312 tests pass
Original prompt

This section details on the original issue you should resolve

<issue_title>[js-api-parser] Constructor not rendered properly</issue_title>
<issue_description>Reported as part of another PR

I was looking at the output for ts-http-runtime and noticed the Sanitizer constructor renders as:

constructor({ additionalAllowedHeaderNames: allowedHeaderNames, additionalAllowedQueryParameters: allowedQueryParameters, }?: , input?: SanitizerOptions);

which is obviously not valid TypeScript. The .api.md renders it correctly as constructor(input?: SanitizerOptions).

The issue is that API Extractor reports destructured parameters in a kind of weird way - the parameters array has two entries: 1. one with the raw destructuring pattern as the name (and an empty type token range),
2. and then a synthetic input parameter with the actual SanitizerOptions type.

The constructor generator just iterates item.parameters and emits each one, so it dumps the destructuring pattern as-is, adds ?: with no type, then a comma, then the real input parameter.

I don't think this is a regression from this PR specifically - the constructor generator has always used item.parameters - but removing the fallback does mean there's no safety net for cases like this.

I think the fix could be pretty simple - just skip parameters where the name starts with { (since the synthetic normalized parameter always follows with the actual type info). Something like:

if (param.name.startsWith("{")) {
  continue;
}

Feel free to push this out to a separate issue if you'd rather not scope-creep this PR

Originally posted by @maorleger in #14376 (comment)
</issue_description>

Comments on the Issue (you are @copilot in this section)


🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

Copilot AI changed the title [WIP] Fix constructor rendering for Sanitizer in js-api-parser [js-api-parser] Fix constructor rendering for destructured parameters Mar 10, 2026
@KarishmaGhiya
Copy link
Member

@copilot resolve merge conflicts

Copy link
Contributor Author

Copilot AI commented Mar 11, 2026

@copilot resolve merge conflicts

Resolved in 17520fd — synced all js-api-parser files with main (which included the TokenCollector refactoring from #14376) and rebased the destructured parameter fix on top. All 312 tests pass.

@KarishmaGhiya
Copy link
Member

@copilot Rebase the branch with the latest on main and resolve the merge conflicts.

Copy link
Contributor Author

Copilot AI commented Mar 11, 2026

@copilot Rebase the branch with the latest on main and resolve the merge conflicts.

Done in bd9adce — synced the branch with the latest main (minimatch bump, sdk-testgen ESLint migration to flat config, azsdk-cli codeowners updates, and dependency bumps across tsp-client, js-sdk-release-tools, typespec-bump-deps). Only the constructor fix remains as a delta from main. All 312 tests pass.

@KarishmaGhiya KarishmaGhiya force-pushed the copilot/fix-sanitzer-constructor-rendering branch from bd9adce to f84b302 Compare March 11, 2026 09:40
@KarishmaGhiya KarishmaGhiya marked this pull request as ready for review March 11, 2026 09:40
Copilot AI review requested due to automatic review settings March 11, 2026 09:40
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the JS APIView parser (js-api-parser) to avoid emitting invalid constructor signatures when API Extractor reports destructured parameters, and adds a unit test to cover the scenario. It also includes a package-lock.json refresh from rebasing/syncing with main.

Changes:

  • Skip destructured constructor parameters (names starting with {) so only the synthetic normalized parameter is rendered.
  • Add a unit test covering the destructured-parameter constructor case.
  • Update package-lock.json with refreshed dependency resolutions.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

File Description
tools/apiview/parsers/js-api-parser/src/tokenGenerators/constructor.ts Filters out destructured parameters before token emission.
tools/apiview/parsers/js-api-parser/test/tokenGenerators/constructor.spec.ts Adds a test to ensure destructured parameters are skipped.
tools/apiview/parsers/js-api-parser/package-lock.json Updates dependency lockfile after rebase/sync.
Files not reviewed (1)
  • tools/apiview/parsers/js-api-parser/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)

tools/apiview/parsers/js-api-parser/package-lock.json:2739

  • rimraf@6.1.2 (and its dependency glob@13) declares engines.node as 20 || >=22. The js-api-parser CI pipeline is configured to use Node 18.x, and the build script runs rimraf via npm run clean, so this lockfile update risks breaking CI/builds under Node 18. Consider pinning rimraf/glob to versions that support Node 18, or updating the pipeline/tool’s supported Node version to 20+ and documenting it.

KarishmaGhiya and others added 5 commits March 18, 2026 19:15
…ring

Co-authored-by: KarishmaGhiya <8968058+KarishmaGhiya@users.noreply.github.com>
…uctor.ts

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ic excerptText

Co-authored-by: KarishmaGhiya <8968058+KarishmaGhiya@users.noreply.github.com>
@KarishmaGhiya KarishmaGhiya force-pushed the copilot/fix-sanitzer-constructor-rendering branch from a358d8e to 20b6825 Compare March 19, 2026 02:16
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.

[js-api-parser] Constructor not rendered properly

3 participants