fix: allow excess protocol-specific fields on Binding (issue #735) - #1228
fix: allow excess protocol-specific fields on Binding (issue #735)#1228tzh476 wants to merge 7 commits into
Conversation
…#735) Change-Id: I2b53925665dd23e273e90fe42eb8ee90b4342907
🦋 Changeset detectedLatest commit: 283aa91 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
Change-Id: Ie4f192783e7f7fb3e533f058974c6a5bb51a3eb8
|
Hi maintainers - friendly ping: this PR (fixes #735, binding excess properties) is ready and MERGEABLE, changeset added, CI/SonarCloud passing; happy to adjust on review. Thank you. |
…ture
The fix in this PR is a change to an interface, and jest does not type-check,
so the runtime test in `bindings.spec.ts` passes both with and without it -- it
exercises `BindingV2`'s accessors, not the declaration. Nothing in the suite
observed the actual change.
`tsc` does, and this repository already runs it via `build:esm`. Control
experiment on a tree with the index signature removed:
test/models/v2/bindings-types.spec.ts(27,7): error TS2322 ... 'groupId' does not exist in type 'Binding'
test/models/v2/bindings-types.spec.ts(31,18): error TS2339 Property 'groupId' does not exist on type 'Binding'
test/models/v2/bindings-types.spec.ts(38,7): error TS2322 ... v3 Binding
test/models/v2/bindings-types.spec.ts(41,17): error TS2339 Property 'is' does not exist on type 'Binding'
With the signature in place: 0 errors. So removing it now breaks the build here
instead of silently regressing for everyone writing a protocol binding.
The third case uses `@ts-expect-error` on `bindingVersion: 42` to pin the other
direction: opening up excess properties must not also swallow a wrong type on a
*declared* member. If it ever did, the directive would become unused and tsc
would fail -- which is the intent.
Covers both v2 and v3, since the PR changed both.
Change-Id: I959877fadbf4fc4b70ed8b4b92c084af9f335d69
Disclosure: prepared by a human working with an LLM assistant.
|
Pushed The fix here changes an interface, and jest does not type-check — so the runtime test in
With the signature in place: 0 errors. So if it is ever dropped, the build fails here instead of silently regressing for everyone writing a protocol binding. Covers v2 and v3 (the PR touches both), using realistic shapes — Kafka Current state: Happy to drop the type test into its own PR, or reshape it if you have a preferred convention for type assertions — I could not find an existing one in the repo. Disclosure: human + LLM collaboration; the numbers above are from runs on this branch. |
The docstring cited TS2322 for the control experiment on the unfixed tree. Re-running that experiment with a standalone tsc gives TS2353: control.ts(6,3): error TS2353: Object literal may only specify known properties, and 'groupId' does not exist in type 'Binding'. TS2322 is the general 'not assignable' error; TS2353 is specifically the excess-property check, which is the one this fix is about. Comment only, no behaviour change. Change-Id: Id862dd73dfc036f01b1de88ef1b9a1069da7ee8a
|
Ran the control experiment this PR's test claims, and it corrects an error code I stated twice above I could not So the substance holds: a Kafka binding with What I got wrong: I wrote Restating the reason this type-level test exists, since it is the unusual part of the PR: the fix is a Scope note: this verifies the type behaviour. I have not run the repo's jest suite locally, for the |
|
Process note rather than a review ping: the test workflow on this PR has never been allowed to run. Same workflows, same repo. The difference is that this is a first-time-contributor PR, so GitHub holds Worth flagging because of what it does to the signal: the SonarCloud quality gate passing is not What I did verify without CI, since this PR is a type-only change and jest does not type-check: Standalone Nothing else needed from me — I will not follow up again on either PR. |
My own comment on this PR was wrong, and the test it described could not
fail.
The type test asserts that `Binding` accepts protocol-specific fields, and
its docstring said the fix is observed by `tsc` "which this repository
already runs via `build:esm`", so removing the index signature would break
the build.
Measured instead of assumed:
* `build:esm` is plain `tsc`, and `packages/parser/tsconfig.json` sets
`"include": ["src"]`. `tsc --listFiles` does not list this test file, so
the test tree is never type-checked.
* Removing the index signature from `spec-types/v2.ts` therefore produces
no build error at all, and jest still passes 3/3 because Babel strips
types without checking them. The test was inert in both directions.
* Type-checking the file directly DOES catch it, with `TS2322` and
`TS2339` -- not the `TS2353` the comment quoted.
So this adds `tsconfig.tests.json` (extends the package config, `noEmit`,
includes `src` plus this spec) and a `type-check:tests` script. Verified as
a real guard rather than described as one:
npm run type-check:tests -> exit 0 on this branch
remove the v2 index signature -> TS2322 + TS2339, non-zero exit
restore it -> exit 0 again
Regression checks: `tsc -p tsconfig.json --noEmit` still exits 0, so the
published build is unaffected, and `jest test/models/v2/` is 648 passed /
36 suites.
The docstring now states what the tooling actually does, including that an
earlier revision of it claimed a build failure that could not happen.
Change-Id: Ia4283679aa8670fefac53ff0ca7505bdc2b10c9e
|
1 participant
Fixes #735.
Adds a
[propName: string]: any;index signature to the v2 and v3Bindingspec-type interfaces (per the maintainer-approved approach), so protocol-specific binding fields such asclientIdare accessible without a cast.Binding.Verified: tsc --noEmit clean; v2 bindings 9/9; v3 bindings 8/8.