fix: removes the proxy and adds a polling mechanism for js-core package#44
fix: removes the proxy and adds a polling mechanism for js-core package#44pandeymangg wants to merge 4 commits intomainfrom
Conversation
WalkthroughThis pull request refactors the formbricks SDK from a proxy-based dynamic dispatch pattern to explicit method implementations. The primary export object transitions from using a Proxy handler that routed calls to 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/js/src/lib/load-formbricks.test.ts (1)
8-19:⚠️ Potential issue | 🟡 Minor
setNoncemethod missing frommockFormbricks.The
mockFormbricksobject includes all methods exceptsetNonce, which is part of theTFormbricksinterface. IfcallMethod("setNonce", ...)is tested, this mock will be incomplete.🔧 Add missing setNonce mock
const mockFormbricks = { setup: vi.fn(), init: vi.fn(), track: vi.fn(), setEmail: vi.fn(), setAttribute: vi.fn(), setAttributes: vi.fn(), setLanguage: vi.fn(), setUserId: vi.fn(), + setNonce: vi.fn(), logout: vi.fn(), registerRouteChange: vi.fn(), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/js/src/lib/load-formbricks.test.ts` around lines 8 - 19, The test mock object mockFormbricks is missing the setNonce method required by the TFormbricks interface, so add a setNonce: vi.fn() entry to mockFormbricks (used by tests that call callMethod("setNonce", ...)) to make the mock complete and avoid undefined method errors when testing setNonce behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/js/src/index.ts`:
- Around line 14-25: Add a backwards-compatible init method to the exported
formbricks object so tests and callers can call formbricks.init(initConfig);
specifically, add an init: (initConfig) => callMethod("init", initConfig) entry
alongside the existing methods (same pattern as setEmail, setup, track, etc.)
and mark it as deprecated in comments if desired; this ensures
callMethod("init", ...) is invoked when formbricks.init is called and prevents
the runtime test failures.
In `@packages/js/src/lib/load-formbricks.test.ts`:
- Around line 396-403: The current cleanup in the concurrent setup test uses
mockSetTimeoutImmediate(), Promise.race([firstSetup, ...]) and swapping
globalThis.setTimeout back to originalSetTimeout which can leave timers in an
inconsistent state; replace that pattern by using the test runner's fake timers
and deterministically advancing them (e.g., call vi.useFakeTimers() at start of
the test, kick off firstSetup, then await vi.advanceTimersByTimeAsync(...) to
force the pending timeout to run, await firstSetup, and finally call
vi.useRealTimers() to restore normal timer behavior), ensuring you remove the
Promise.race and direct swapping of originalSetTimeout and reference the same
symbols (mockSetTimeoutImmediate, firstSetup, originalSetTimeout,
globalThis.setTimeout) when editing the test.
In `@packages/js/src/lib/load-formbricks.ts`:
- Around line 146-148: coreInstance is assigned before awaiting
coreInstance.setup(...), so if setup() throws the instance remains set and
callMethod may operate on an uninitialized core; wrap the setup call in a
try/catch (or assign coreInstance only after successful setup) so that on
failure you clear/reset coreInstance (e.g., set to undefined/null) and rethrow
the error, and only call processQueue() after setup succeeds; update the logic
around coreInstance, setup, processQueue, and any error paths to ensure
coreInstance is not left assigned when setup fails.
- Around line 9-69: The loadFormbricksSDK function can append duplicate <script>
tags on repeated calls after a failure; fix it by adding a module-scoped
sentinel (e.g., let formbricksScriptAppended = false) or checking for an
existing script element by src before creating/appending a new element, use that
sentinel/check in loadFormbricksSDK to return early if already appended, and
ensure the sentinel is cleared or updated appropriately on permanent
failures/success so subsequent legitimate retries behave correctly; reference
the loadFormbricksSDK function and the created script element handling
(script.src, document.head.appendChild) when implementing the guard.
---
Outside diff comments:
In `@packages/js/src/lib/load-formbricks.test.ts`:
- Around line 8-19: The test mock object mockFormbricks is missing the setNonce
method required by the TFormbricks interface, so add a setNonce: vi.fn() entry
to mockFormbricks (used by tests that call callMethod("setNonce", ...)) to make
the mock complete and avoid undefined method errors when testing setNonce
behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ad5cccc-bf84-4ea5-90d7-eb8b3913600c
📒 Files selected for processing (5)
packages/js/src/index.test.tspackages/js/src/index.tspackages/js/src/lib/load-formbricks.test.tspackages/js/src/lib/load-formbricks.tspackages/js/src/types/formbricks.ts
💤 Files with no reviewable changes (1)
- packages/js/src/types/formbricks.ts
|
1 participant
Problem: The
@formbricks/jswrapper SDK uses aProxyto intercept all method calls and route them to the dynamically loadedformbricks.umd.cjscore. After loading the core via<script>tag, it readsglobalThis.formbricks— which is set by the UMD wrapper. If the UMD environment detection is fooled (e.g. another script on the page leaksexportsormoduleinto the global scope), the assignment goes to
module.exportsinstead ofglobalThis.formbricks, and initialization fails with"Failed to load Formbricks SDK".Changes:
Replace
Proxywith explicit method object (index.ts): Each SDK method (setup,track,setEmail, etc.) is now a direct function that delegates tosetup()orcallMethod(). This makes the API statically typed, debuggable (real function names in stack traces), and catches typos at compile time instead of silently queueing them.Add polling fallback after
script.onload(load-formbricks.ts): After the core script loads, ifglobalThis.formbricksisn't set immediately (UMD detection was fooled), pollsevery 10ms for up to 500ms before giving up. This pairs with the explicit
globalThisassignment being added to js-core on the server side.Store
coreInstancelocally (load-formbricks.ts): The loaded core SDK reference is captured once during setup rather than re-readingglobalThis.formbrickson every method call.More reliable and avoids issues if the global is later overwritten.
Preserve method queueing: Methods called before
setup()are still queued and flushed after initialization, same as before.Add UMD fallback tests (
load-formbricks.test.ts): Two new tests simulate the UMD detection failure scenario — one where the global appears after a delay (polling recovers), andone where it never appears (polling times out gracefully).
Add queue-test playground page (
apps/playground/app/queue-test/page.tsx): Interactive page at/queue-testto manually verify method queueing. Call SDK methods before setup, thentrigger setup and observe the queued calls flush.
Remove deprecated
initmethod (types/formbricks.ts): Theinitmethod (deprecated in favor ofsetup) has been removed from the type interface.