Skip to content

fix(types): retain dependencies of public declarations - #517

Closed
thilllon wants to merge 2 commits into
pubnub:masterfrom
thilllon:fix/public-declaration-dependencies
Closed

thilllon wants to merge 2 commits into
pubnub:masterfrom
thilllon:fix/public-declaration-dependencies

Conversation

@thilllon

Copy link
Copy Markdown
Contributor

PubNub 13.0.1's published declaration bundle fails a strict consumer typecheck with skipLibCheck: false: setProxy() references an undeclared NodeTransportProxyConfiguration, and SubscriptionResponse references an undeclared DataSyncEvent.

The source compiler succeeds because both types exist before declaration generation. The aggregator omits the entire internal Node transport module containing the proxy configuration, while stripInternal removes DataSyncEvent even though the public response union needs it.

  • Move the existing proxy configuration type into the public Node configuration module so the generated method refers to PubNub.NodeTransportProxyConfiguration.
  • Retain DataSyncEvent in declarations, matching the other response variants; its internal fingerprint field remains stripped.
  • Regenerate the tracked declaration bundle and add a Mocha regression test that checks the complete bundle with strict: true and skipLibCheck: false.
  • Declare the already-locked @types/node development dependency directly so the regression check does not rely on transitive npm hoisting.

No runtime behavior or proxy configuration shape changes.

Validation:

  • New regression fails on the original bundle with exactly TS2304 and TS2552, then passes on the regenerated bundle.
  • Node declaration build and the existing types-aggregate.ts pipeline pass. Local pnpm installation used the imported committed npm lockfile; the source compiler was invoked with --types node to avoid the repository's unrelated empty @types/expect stub.
  • 45 related tests pass: Node transport, subscribe event categorisation, event dispatch, and the new declaration regression.
  • Strict .cts and .mts consumers pass under TypeScript 5.8.2 and 7.0.2, including valid/invalid proxy arguments and DataSync event narrowing.
  • Formatting, changed-source ESLint, and git diff --check pass.

Discovered while validating the SDK directly for thilllon/nestjs-kit#505; the reproduction does not depend on NestJS.

@coderabbitai

coderabbitai Bot commented Sep 19, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (8)
  • lib/types/index.d.ts is excluded by !lib/** and included by none
  • package-lock.json is excluded by !**/package-lock.json, !package-lock.json and included by none
  • package.json is excluded by none and included by none
  • src/core/types/api/subscription.ts is excluded by none and included by none
  • src/node/components/configuration.ts is excluded by none and included by none
  • src/node/index.ts is excluded by none and included by none
  • src/transport/node-transport.ts is excluded by none and included by none
  • test/release/declarations.test.ts is excluded by none and included by none

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

⚙️ Run configuration

Configuration used: Repository: pubnub/javascript/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 16a3a1ca-cdf2-48a9-a355-594554c191ad

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pubnub-ops-terraform

pubnub-ops-terraform commented Sep 19, 2026 •

Copy link
Copy Markdown

✅ Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
✅ Open Source Security 0 0 0 0 0 issues
✅ Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@mohitpubnub

Copy link
Copy Markdown
Contributor

Thank you for your effors to create this PR, Let us give some to to review the same.

@parfeon parfeon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@mohitpubnub

Copy link
Copy Markdown
Contributor

@thilllon Due to our repository setup and deployment process, we aren't able to merge PRs directly from external branches.To follow our release workflow while ensuring your contribution is properly recognized, I’ve ported your changes over to PR #518 (with some very minor tweak) to process them through our pipeline.

We’ve kept your commit authorship intact so your contribution is fully visible in git history and changelog.

Thanks again for helping improve the library!

@pubnub-release-bot

Copy link
Copy Markdown
Contributor

@thilllon this issue is addressed in v13.0.3

@github-actions github-actions Bot added the status: done This issue is considered resolved. label Sep 22, 2026
@thilllon

Copy link
Copy Markdown
Contributor Author

@mohitpubnub Thanks!

@thilllon
thilllon deleted the fix/public-declaration-dependencies branch September 25, 2026 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: done This issue is considered resolved.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants