fix(js-sdk): Add CJS-compatible WebSocket loading #1021
Conversation
apps/js-sdk/firecrawl/src/index.ts
Outdated
| return require('isows').WebSocket; | ||
| } catch { | ||
| try { | ||
| return import('isows').then(m => m.WebSocket); |
There was a problem hiding this comment.
Missing an await here, right? Either here or on line 964.
There was a problem hiding this comment.
Thanks for catching that! I've updated the code to properly handle the WebSocket loading. Could you please review the changes again?
There was a problem hiding this comment.
This can cause a race condition. I'd prefer importing it lazily in an already-asynchronous function, like the watch crawl endpoint's function.
apps/js-sdk/firecrawl/src/index.ts
Outdated
| return require('isows').WebSocket; | ||
| } catch { | ||
| try { | ||
| return import('isows').then(m => m.WebSocket); |
There was a problem hiding this comment.
This can cause a race condition. I'd prefer importing it lazily in an already-asynchronous function, like the watch crawl endpoint's function.
There was a problem hiding this comment.
Thank you for the work you've put into this! I believe the best approach here would be to use top-level await for ESM, while keeping the current implementation for CJS. If you're open to merging this, it might be worth exploring how to configure tsup to use distinct implementations for ESM and CJS.
I really appreciate your efforts on this, and I'm happy to assist further if needed. Let me know how I can help!
apps/js-sdk/firecrawl/src/index.ts
Outdated
| this.status = "scraping"; | ||
| this.data = []; | ||
|
|
||
| this.initialized = CrawlWatcher.loadWebSocket() |
There was a problem hiding this comment.
I dont think this is good enough, cause ws is not initialized when the constructor is called but later async, so there can be a race condition.
RutamBhagat
force-pushed
the
fix-cjs-build-error
branch
from
9fb0c05 to
00f6d58
Compare
RutamBhagat
force-pushed
the
fix-cjs-build-error
branch
from
00f6d58 to
67db6a1
Compare
|
Hi Rutam, we're going a different way to fix this, so I'm closing this PR. Thanks for the work. |
Fixes: #1011
The build passes for both ESM and CJS