feat: enableEventEngine, restore set default#495
Conversation
fix titanium test to equate expected operation type
…ubscription_manager component specific, update subscribe tests to in corporate ee changes where EE discard `m` field for handshake, and make presence heartbeat whenever subscribe call made, additional mock request infering to EE http requests
…ault retry policy, which delayes the failure and hits timeout for failure time call test using mock, unsubscribe.test.ts expects no leave call in certain scenario which is changed intentionally with event engine.
📝 WalkthroughWalkthroughEvent Engine defaults were enabled; retry-policy upper-bound validation checks were removed; event-engine state emissions now include operation fields for subscribe/unsubscribe; tests and mocks were updated to reflect Event Engine and retry changes; a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
mohitpubnub
added
priority: high
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/event-engine/states/receiving.ts (1)
131-135:⚠️ Potential issue | 🟡 MinorInconsistency:
receiveFailurestatus has nooperationfield.Every other terminal/transition status in this file now sets
operation(mostlyPNUnsubscribeOperation). ThereceiveFailureemission still omits it, which will surface asundefinedin status consumers that log/display the operation (e.g.,console-logger.ts:346-350). Since the failure originates from a subscribe long-poll,RequestOperation.PNSubscribeOperationis the natural value here.Proposed fix
ReceivingState.on(receiveFailure.type, (context, { payload }) => ReceiveFailedState.with({ ...context, reason: payload }, [ - emitStatus({ category: categoryConstants.PNDisconnectedUnexpectedlyCategory, error: payload.status?.category }), + emitStatus({ + category: categoryConstants.PNDisconnectedUnexpectedlyCategory, + operation: RequestOperation.PNSubscribeOperation, + error: payload.status?.category, + }), ]), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/event-engine/states/receiving.ts` around lines 131 - 135, The receiveFailure status emission is missing the operation field; update the ReceivingState.on handler that calls ReceiveFailedState.with(...) to include operation: RequestOperation.PNSubscribeOperation in the status object (the same place emitStatus(...) is created), so the produced status from receiveFailure contains operation. Modify the handler that references receiveFailure, ReceiveFailedState.with, and emitStatus (with categoryConstants.PNDisconnectedUnexpectedlyCategory) to add operation: RequestOperation.PNSubscribeOperation.
🧹 Nitpick comments (7)
.coderabbit.yaml (1)
88-92: Excludingdist/**andlib/**may hide regressions for a published SDK.For a PubNub JS SDK that ships these directories as the published artifact, excluding them from review means generated/bundled output drift (e.g., accidental source-map leaks, unexpected size/API surface changes from the event-engine default flip in this PR) won't surface in PRs. Consider keeping the exclusions but ensuring a separate CI check gates build-output changes, or narrow the filter (e.g., only exclude minified bundles) so reviewers still see meaningful
dist/libdiffs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.coderabbit.yaml around lines 88 - 92, The path_filters currently exclude "dist/**" and "lib/**", which hides changes to published build artifacts; update .coderabbit.yaml to stop excluding those directories (or narrow the filters to only exclude specific minified bundles like "*.min.js") and instead add/enable a CI check that verifies build-output changes (e.g., a dedicated "verify-dist" job) so reviewers can see meaningful diffs while still gating accidental artifact regressions; modify the path_filters entries (the "!dist/**" and "!lib/**" lines) and ensure CI pipeline rules reference a new verification job to catch build-output drift.test/unit/retry-configuration.test.ts (2)
52-72: Minor:getDelay()assertion not repeated forPubNub.NoneRetryPolicy().The
RetryPolicy.None()path asserts bothshouldRetry() === falseandgetDelay() === -1, but thePubNub.NoneRetryPolicy()path only assertsshouldRetry(). If the intent is to confirm both factories produce equivalent policies, mirror thegetDelay()assertion for completeness.Proposed fix
const fromPubNub = PubNub.NoneRetryPolicy(); assert.doesNotThrow(() => fromPubNub.validate()); // `@ts-expect-error` Intentional access for assertion. assert.strictEqual(fromPubNub.shouldRetry(), false); + // `@ts-expect-error` Intentional access for assertion. + assert.strictEqual(fromPubNub.getDelay(), -1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/retry-configuration.test.ts` around lines 52 - 72, Add the missing assertion to verify that PubNub.NoneRetryPolicy() matches RetryPolicy.None() for delay: after asserting fromPubNub.shouldRetry() === false, also assert fromPubNub.getDelay() === -1; reference the PubNub.NoneRetryPolicy() instance and the methods shouldRetry() and getDelay() to mirror the existing checks for RetryPolicy.None().
15-50: Coverage gap: tests verify validate() no longer throws, but not the removed caps themselves.The PR's stated intent is “removed caps on maximum retry values.”
maximumRetry: 1300does exercise the lifted cap (previous docs noted caps of 6/10), but it would be worth also asserting explicit boundary values that were previously rejected (e.g.,maximumRetry: 11for Linear,maximumRetry: 7andmaximumDelay: 151for Exponential) to lock in the new contract and prevent a regression if caps are reintroduced.Also note: the JSDoc on
validate()(src/core/components/retry-policy.ts:159-167) still documents the old@throwsformaximumRetry > 6 / > 10andmaximumDelay > 150— please update the interface JSDoc to match the relaxed behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/retry-configuration.test.ts` around lines 15 - 50, Add explicit tests that assert previously-capped boundary values now succeed: in test/unit/retry-configuration.test.ts extend the LinearRetryPolicy test (RetryPolicy.LinearRetryPolicy) to include maximumRetry: 11 and assert validate() does not throw and properties match; extend the ExponentialRetryPolicy test (RetryPolicy.ExponentialRetryPolicy) to include maximumRetry: 7 and maximumDelay: 151 and assert validate() does not throw and properties match. Also update the JSDoc for the validate() method in src/core/components/retry-policy.ts (the validate function) to remove the outdated `@throws` clauses about maximumRetry > 6 / > 10 and maximumDelay > 150 so the docs reflect the relaxed constraints.test/integration/operations/unsubscribe.test.ts (1)
19-30: Consider also adding EE-enabled coverage for unsubscribe/leave.Opting this suite out of Event Engine is fine for preserving current assertions, but given EE is now the default and the comment acknowledges EE issues leave calls regardless of heartbeat interval, leave behavior under EE is now the production-default path with no integration coverage here. Consider adding a parallel describe block (or a second client fixture) with
enableEventEngine: trueasserting that/leaveis invoked appropriately onunsubscribe()/unsubscribeAll().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/operations/unsubscribe.test.ts` around lines 19 - 30, Add a parallel EE-enabled integration test to cover unsubscribe/leave behavior: duplicate or add a new describe/it block that creates a PubNub client with enableEventEngine: true (same constructor used where pubnub is instantiated and setHeartbeatInterval(0) is called), then call pubnub.unsubscribe() and pubnub.unsubscribeAll() and assert that the server /leave endpoint is invoked (mock or spy the HTTP/transport layer used by PubNub to capture leave requests). Ensure the new test uses the same fixtures/cleanup as the existing suite but with enableEventEngine: true so unsubscribe/leave behaviour under Event Engine is exercised.src/core/interfaces/configuration.ts (2)
298-308: Consider adding a "since" / "changed in" note to theenableEventEngineJSDoc.The
@defaultwas flipped fromfalse→true, but the surrounding prose (“Whether to use the standardized workflows for subscribe and presence.”) gives no indication this is a new default. Adding a brief “default changed totruein vX.Y.Z” note here (and onrestoreat line 250-255) will help users diagnose behavior changes after upgrading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/interfaces/configuration.ts` around lines 298 - 308, Update the JSDoc for enableEventEngine to include a short "since/changed in" note indicating the default was flipped to true (e.g., "Default changed to `true` in vX.Y.Z"), and make the same addition to the JSDoc for the restore option (the block around `restore`) so both entries explicitly document the version when the default changed; reference the `enableEventEngine` and `restore` symbols when editing their comment blocks and keep the note concise.
26-26: Breaking default flip — ensure CHANGELOG / migration notes cover this.Flipping
RESTOREandENABLE_EVENT_ENGINEtotrueis a behavioral breaking change for existing callers that omit these flags:
enableEventEngine=truetriggers automatic exponential retry configuration (src/core/components/configuration.ts:93-107), presence state maintenance (pubnub-common.ts:2628-2631), and the new EE subscription loop (pubnub-common.ts:453-480) — including presence heartbeats whenheartbeatIntervalis set.restore=truechanges web network-down behavior fromdestroy()todisconnect()(src/web/index.ts:228-232).Recommend: (a) add a
BREAKING CHANGE:entry to the CHANGELOG and release notes, (b) call out in the TSDoc ofenableEventEngine/restorethat the default changed in this release, and (c) consider logging a one-time info message at client init summarizing the active subscription engine to aid debugging for users upgrading silently.Also applies to: 52-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/interfaces/configuration.ts` at line 26, The default flags RESTORE and ENABLE_EVENT_ENGINE were flipped to true which is a breaking behavioral change; update project docs and runtime visibility: add a "BREAKING CHANGE:" entry to the CHANGELOG/release notes describing the default changes and their behavioral effects (reference enableEventEngine behavior in src/core/components/configuration.ts and restore behavior in src/web/index.ts), update the TSDoc for the public configuration properties (enableEventEngine and restore) to explicitly state the new defaults and migration guidance, and add a one-time info log at client initialization (where the client picks the subscription engine) that prints the active subscription engine and the values of enableEventEngine/restore to help users detect silent upgrades.test/integration/endpoints/subscribe.test.ts (1)
434-483: Nit: extract a helper for the repeatedee/handshake/long-poll/heartbeat mock triad.The same
handshake → persistent receiveMessages → /heartbeat$pattern is now duplicated across five tests in this file (lines ~61-96, 123-160, 188-225, 250-287, 438-483). This interval/delta test only diverges on a second cursor. Consider a small test helper (or extendingutils.createSubscribeMockScopes) that accepts{ channels, channelGroups, filterExpression, cursors[] }and sets up all three interceptors, to reduce noise in future test additions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/endpoints/subscribe.test.ts` around lines 434 - 483, Extract a test helper (e.g., utils.createSubscribeMockScopes or a new helper) that encapsulates the repeated handshake → long-poll (receiveMessages) → persistent empty response → heartbeat nock setup used in the "presence listener called for interval / delta update" test and the four other tests; the helper should accept parameters like channels/channelGroups/filterExpression and an ordered cursors array (e.g., [cursorHandshake, cursorAfterInterval]) and use utils.createNock() to register the three GET interceptors (the initial '/v2/subscribe/.../0' handshake returning the first cursor, the subsequent '/v2/subscribe/.../0' return with messages using the next cursor(s), and the persistent empty reply for later polls) plus the '/heartbeat$' mock, then replace the duplicated blocks in the test with a single call to this helper (refer to the existing usages of utils.createNock(), the test name "presence listener called for interval / delta update", and the cursor variables cursorHandshake/cursorAfterInterval to locate where to swap in the helper).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.coderabbit.yaml:
- Around line 87-105: The path_filters block is defined at top-level so it is
ignored; move the existing path_filters mapping under the reviews key (i.e.,
make reviews: { path_filters: [...] }) so the exclusions (path patterns like
"!dist/**", "!lib/**", "!node_modules/**", "!package-lock.json", "!.pubnub.yml",
"!.vscode/**") are applied; after moving, remove the redundant "!lib/types/**"
entry since "!lib/**" already covers it.
---
Outside diff comments:
In `@src/event-engine/states/receiving.ts`:
- Around line 131-135: The receiveFailure status emission is missing the
operation field; update the ReceivingState.on handler that calls
ReceiveFailedState.with(...) to include operation:
RequestOperation.PNSubscribeOperation in the status object (the same place
emitStatus(...) is created), so the produced status from receiveFailure contains
operation. Modify the handler that references receiveFailure,
ReceiveFailedState.with, and emitStatus (with
categoryConstants.PNDisconnectedUnexpectedlyCategory) to add operation:
RequestOperation.PNSubscribeOperation.
---
Nitpick comments:
In @.coderabbit.yaml:
- Around line 88-92: The path_filters currently exclude "dist/**" and "lib/**",
which hides changes to published build artifacts; update .coderabbit.yaml to
stop excluding those directories (or narrow the filters to only exclude specific
minified bundles like "*.min.js") and instead add/enable a CI check that
verifies build-output changes (e.g., a dedicated "verify-dist" job) so reviewers
can see meaningful diffs while still gating accidental artifact regressions;
modify the path_filters entries (the "!dist/**" and "!lib/**" lines) and ensure
CI pipeline rules reference a new verification job to catch build-output drift.
In `@src/core/interfaces/configuration.ts`:
- Around line 298-308: Update the JSDoc for enableEventEngine to include a short
"since/changed in" note indicating the default was flipped to true (e.g.,
"Default changed to `true` in vX.Y.Z"), and make the same addition to the JSDoc
for the restore option (the block around `restore`) so both entries explicitly
document the version when the default changed; reference the `enableEventEngine`
and `restore` symbols when editing their comment blocks and keep the note
concise.
- Line 26: The default flags RESTORE and ENABLE_EVENT_ENGINE were flipped to
true which is a breaking behavioral change; update project docs and runtime
visibility: add a "BREAKING CHANGE:" entry to the CHANGELOG/release notes
describing the default changes and their behavioral effects (reference
enableEventEngine behavior in src/core/components/configuration.ts and restore
behavior in src/web/index.ts), update the TSDoc for the public configuration
properties (enableEventEngine and restore) to explicitly state the new defaults
and migration guidance, and add a one-time info log at client initialization
(where the client picks the subscription engine) that prints the active
subscription engine and the values of enableEventEngine/restore to help users
detect silent upgrades.
In `@test/integration/endpoints/subscribe.test.ts`:
- Around line 434-483: Extract a test helper (e.g.,
utils.createSubscribeMockScopes or a new helper) that encapsulates the repeated
handshake → long-poll (receiveMessages) → persistent empty response → heartbeat
nock setup used in the "presence listener called for interval / delta update"
test and the four other tests; the helper should accept parameters like
channels/channelGroups/filterExpression and an ordered cursors array (e.g.,
[cursorHandshake, cursorAfterInterval]) and use utils.createNock() to register
the three GET interceptors (the initial '/v2/subscribe/.../0' handshake
returning the first cursor, the subsequent '/v2/subscribe/.../0' return with
messages using the next cursor(s), and the persistent empty reply for later
polls) plus the '/heartbeat$' mock, then replace the duplicated blocks in the
test with a single call to this helper (refer to the existing usages of
utils.createNock(), the test name "presence listener called for interval / delta
update", and the cursor variables cursorHandshake/cursorAfterInterval to locate
where to swap in the helper).
In `@test/integration/operations/unsubscribe.test.ts`:
- Around line 19-30: Add a parallel EE-enabled integration test to cover
unsubscribe/leave behavior: duplicate or add a new describe/it block that
creates a PubNub client with enableEventEngine: true (same constructor used
where pubnub is instantiated and setHeartbeatInterval(0) is called), then call
pubnub.unsubscribe() and pubnub.unsubscribeAll() and assert that the server
/leave endpoint is invoked (mock or spy the HTTP/transport layer used by PubNub
to capture leave requests). Ensure the new test uses the same fixtures/cleanup
as the existing suite but with enableEventEngine: true so unsubscribe/leave
behaviour under Event Engine is exercised.
In `@test/unit/retry-configuration.test.ts`:
- Around line 52-72: Add the missing assertion to verify that
PubNub.NoneRetryPolicy() matches RetryPolicy.None() for delay: after asserting
fromPubNub.shouldRetry() === false, also assert fromPubNub.getDelay() === -1;
reference the PubNub.NoneRetryPolicy() instance and the methods shouldRetry()
and getDelay() to mirror the existing checks for RetryPolicy.None().
- Around line 15-50: Add explicit tests that assert previously-capped boundary
values now succeed: in test/unit/retry-configuration.test.ts extend the
LinearRetryPolicy test (RetryPolicy.LinearRetryPolicy) to include maximumRetry:
11 and assert validate() does not throw and properties match; extend the
ExponentialRetryPolicy test (RetryPolicy.ExponentialRetryPolicy) to include
maximumRetry: 7 and maximumDelay: 151 and assert validate() does not throw and
properties match. Also update the JSDoc for the validate() method in
src/core/components/retry-policy.ts (the validate function) to remove the
outdated `@throws` clauses about maximumRetry > 6 / > 10 and maximumDelay > 150 so
the docs reflect the relaxed constraints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c18119e0-e4d5-48b6-8283-3f81d6e47741
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsontest/dist/web-titanium.test.jsis excluded by!**/dist/**
📒 Files selected for processing (10)
.coderabbit.yamlsrc/core/components/retry-policy.tssrc/core/interfaces/configuration.tssrc/event-engine/states/handshaking.tssrc/event-engine/states/receiving.tstest/integration/components/subscription_manager.test.tstest/integration/endpoints/subscribe.test.tstest/integration/endpoints/time.test.tstest/integration/operations/unsubscribe.test.tstest/unit/retry-configuration.test.ts
💤 Files with no reviewable changes (1)
- src/core/components/retry-policy.ts
| # Path filters - ignore generated/vendored/build output | ||
| path_filters: | ||
| # Build outputs | ||
| - "!dist/**" | ||
| - "!lib/**" | ||
| - "!upload/**" | ||
|
|
||
| # Dependencies | ||
| - "!node_modules/**" | ||
|
|
||
| # Lock files - too noisy, low review value | ||
| - "!package-lock.json" | ||
|
|
||
| # Config/metadata that rarely needs review | ||
| - "!.pubnub.yml" | ||
| - "!.vscode/**" | ||
|
|
||
| # Generated type bundles | ||
| - "!lib/types/**" |
There was a problem hiding this comment.
path_filters is at the wrong nesting level — filters will not apply.
Per the CodeRabbit configuration schema, path_filters lives under reviews, not at the top level. As written here, these exclusions are effectively ignored, so dist/**, lib/**, node_modules/**, package-lock.json, etc. will still be pulled into reviews — defeating the intent of this block.
🛠️ Proposed fix: move path_filters under reviews
# Tools configuration
tools:
# Enable GitHub checks integration
github-checks:
enabled: true
timeout_ms: 120000
+ # Path filters - ignore generated/vendored/build output
+ path_filters:
+ # Build outputs
+ - "!dist/**"
+ - "!lib/**"
+ - "!upload/**"
+ # Dependencies
+ - "!node_modules/**"
+ # Lock files - too noisy, low review value
+ - "!package-lock.json"
+ # Config/metadata that rarely needs review
+ - "!.pubnub.yml"
+ - "!.vscode/**"
+ # Generated type bundles
+ - "!lib/types/**"
+
chat:
auto_reply: true
knowledge_base:
mcp:
usage: enabled
-
-# Path filters - ignore generated/vendored/build output
-path_filters:
- # Build outputs
- - "!dist/**"
- - "!lib/**"
- - "!upload/**"
-
- # Dependencies
- - "!node_modules/**"
-
- # Lock files - too noisy, low review value
- - "!package-lock.json"
-
- # Config/metadata that rarely needs review
- - "!.pubnub.yml"
- - "!.vscode/**"
-
- # Generated type bundles
- - "!lib/types/**"Also worth noting: !lib/** already covers !lib/types/**, so the last entry is redundant once relocated.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Path filters - ignore generated/vendored/build output | |
| path_filters: | |
| # Build outputs | |
| - "!dist/**" | |
| - "!lib/**" | |
| - "!upload/**" | |
| # Dependencies | |
| - "!node_modules/**" | |
| # Lock files - too noisy, low review value | |
| - "!package-lock.json" | |
| # Config/metadata that rarely needs review | |
| - "!.pubnub.yml" | |
| - "!.vscode/**" | |
| # Generated type bundles | |
| - "!lib/types/**" | |
| # Path filters - ignore generated/vendored/build output | |
| path_filters: | |
| # Build outputs | |
| - "!dist/**" | |
| - "!lib/**" | |
| - "!upload/**" | |
| # Dependencies | |
| - "!node_modules/**" | |
| # Lock files - too noisy, low review value | |
| - "!package-lock.json" | |
| # Config/metadata that rarely needs review | |
| - "!.pubnub.yml" | |
| - "!.vscode/**" | |
| # Generated type bundles | |
| - "!lib/types/**" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.coderabbit.yaml around lines 87 - 105, The path_filters block is defined at
top-level so it is ignored; move the existing path_filters mapping under the
reviews key (i.e., make reviews: { path_filters: [...] }) so the exclusions
(path patterns like "!dist/**", "!lib/**", "!node_modules/**",
"!package-lock.json", "!.pubnub.yml", "!.vscode/**") are applied; after moving,
remove the redundant "!lib/types/**" entry since "!lib/**" already covers it.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/core/interfaces/configuration.js`:
- Line 40: Add an explicit "BREAKING CHANGE" entry to CHANGELOG.md describing
the default flip of enableEventEngine (previously false, now true) and link to a
new migration guide that shows how to opt out by setting enableEventEngine:
false; mention the three behavioral cascades (subscription loop switching to
event-engine workflow, ExponentialRetryPolicy being auto-applied when
enableEventEngine is true and no retryConfiguration is provided, and presence
state auto-maintenance during subscribe calls) so users can assess impact, and
add a short migration recipe showing the exact config snippet to set
enableEventEngine: false and how to retain previous retry/presence behavior
referencing ENABLE_EVENT_ENGINE / enableEventEngine, ExponentialRetryPolicy, and
subscribe/SubscriptionSet semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bdea488e-1318-4ef2-8738-09c6ed89f5cc
⛔ Files ignored due to path filters (2)
dist/web/pubnub.jsis excluded by!**/dist/**dist/web/pubnub.min.jsis excluded by!**/dist/**,!**/*.min.js
📒 Files selected for processing (5)
lib/core/components/retry-policy.jslib/core/interfaces/configuration.jslib/event-engine/states/handshaking.jslib/event-engine/states/receiving.jslib/types/index.d.ts
💤 Files with no reviewable changes (1)
- lib/core/components/retry-policy.js
✅ Files skipped from review due to trivial changes (1)
- lib/types/index.d.ts
| * Whether subscription event engine should be used or not. | ||
| */ | ||
| const ENABLE_EVENT_ENGINE = false; | ||
| const ENABLE_EVENT_ENGINE = true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check that tests and docs were updated for the new default, and look for any spots
# that still hardcode or assume enableEventEngine=false as the default.
rg -nP -C2 'enableEventEngine' --type=ts --type=js -g '!**/node_modules/**' -g '!dist/**'
rg -nP -C2 'ENABLE_EVENT_ENGINE\s*=' -g '!**/node_modules/**'
fd -i 'changelog|migration' -t f | head -20Repository: pubnub/javascript
Length of output: 18640
🏁 Script executed:
# Check CHANGELOG.md for BREAKING CHANGE note about ENABLE_EVENT_ENGINE
head -100 CHANGELOG.md | grep -A 5 -B 5 -i "breaking\|event.engine"
# Look for migration guides
fd -i 'migration\|upgrade\|guide' -t f | head -20
# Check if lib/ files are generated (look for build scripts)
cat -n package.json | head -50
# Compare timestamps of src and lib files to verify regeneration
ls -la src/core/interfaces/configuration.ts lib/core/interfaces/configuration.js 2>/dev/null || echo "Files not found"
stat src/core/interfaces/configuration.ts lib/core/interfaces/configuration.js 2>/dev/null | grep -E "File|Modify"Repository: pubnub/javascript
Length of output: 4667
🏁 Script executed:
# Search entire CHANGELOG for enableEventEngine or event engine breaking change
grep -i "enableeventengine\|event.engine" CHANGELOG.md | head -20
# Search more broadly for breaking changes in recent versions
grep -B2 -A5 "BREAKING CHANGE" CHANGELOG.md | head -40
# Look for docs directory with migration/guide info
fd -i 'docs' -t d | head -10
# Check if there's a docs directory with migration guides
find . -type f -name "*.md" -path "*/docs/*" 2>/dev/null | grep -i "migrat\|upgrade\|guide\|breaking" | head -20
# Count tests with enableEventEngine: true vs false
echo "=== Tests with enableEventEngine: true ==="
rg 'enableEventEngine:\s*true' --type=ts --type=js test/ -c
echo "=== Tests with enableEventEngine: false ==="
rg 'enableEventEngine:\s*false' --type=ts --type=js test/ -cRepository: pubnub/javascript
Length of output: 2212
Add explicit "BREAKING CHANGE" documentation for enableEventEngine default flip in CHANGELOG and migration guide.
The three behavioral cascades identified are correctly implemented:
- Subscription loop switches to event-engine workflow when enabled
ExponentialRetryPolicyauto-applied whenenableEventEngine: trueand noretryConfigurationsupplied- Presence state auto-maintained during subscribe calls
Tests provide coverage for both enableEventEngine: true and false modes across event engine, shared-worker, and subscription manager suites. The lib/ artifacts are properly regenerated from src/ via the build process.
However, CHANGELOG.md lacks an explicit "BREAKING CHANGE" note for this default flip—recent entries document other breaking changes (pagination, SubscriptionSet, objects v1 removal) but not this one. A migration guide with the opt-out recipe (enableEventEngine: false) is also not present in the repository.
Add these documentation updates to surface the behavioral change to users upgrading to this version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/core/interfaces/configuration.js` at line 40, Add an explicit "BREAKING
CHANGE" entry to CHANGELOG.md describing the default flip of enableEventEngine
(previously false, now true) and link to a new migration guide that shows how to
opt out by setting enableEventEngine: false; mention the three behavioral
cascades (subscription loop switching to event-engine workflow,
ExponentialRetryPolicy being auto-applied when enableEventEngine is true and no
retryConfiguration is provided, and presence state auto-maintenance during
subscribe calls) so users can assess impact, and add a short migration recipe
showing the exact config snippet to set enableEventEngine: false and how to
retain previous retry/presence behavior referencing ENABLE_EVENT_ENGINE /
enableEventEngine, ExponentialRetryPolicy, and subscribe/SubscriptionSet
semantics.
2 participants
feat(event-engine):
enableEventEnginenow defaults totrue(subscription uses eventEngine workflow),restorenow defaults totrue.Default values for
enableEventEngineandrestoreflags set totrue.BREAKING CHANGES:
enableEventEnginedefaults totrue(subscription will use event engine workflow by default),restoredefaults totrue.refactor(retryConfiguration): removed validation caps on maximumRetry for linear and exponential policies.
removed value caps on maximumRetry for linear and exponential policies. so callers may set arbitrarily large limits (subject to runtime behavior and operational constraints).