-
Notifications
You must be signed in to change notification settings - Fork 44
Avoid calling require('lz4') if it's really not required #316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This changes the utlity module to return a function instead of the module directly. This way, caller can control when the module is actually tried to be resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR modifies the LZ4 module loading strategy to use lazy initialization instead of immediate evaluation. The lz4.ts utility now exports a getter function rather than directly exporting the module, preventing require('lz4') from being called until the module is actually needed. This eliminates console warnings when LZ4 compression is disabled.
Key Changes:
- Converted the LZ4 module export from direct module resolution to a lazy-loading function pattern
- Updated all call sites to invoke the getter function instead of directly accessing the module
- Added memoization to cache the resolution result (including failures) to avoid repeated resolution attempts
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/utils/lz4.ts | Changed from immediate module resolution to lazy-loading with memoization via getResolvedModule() function |
| lib/result/CloudFetchResultHandler.ts | Updated to call LZ4() function instead of accessing LZ4 directly in compression checks and decode operations |
| lib/result/ArrowResultHandler.ts | Updated to call LZ4() function instead of accessing LZ4 directly in compression checks and decode operations |
| lib/DBSQLSession.ts | Updated to call LZ4() function in the LZ4 capability check |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (resolvedModule === undefined) { | ||
| resolvedModule = tryLoadLZ4Module() ?? null; | ||
| } | ||
| return resolvedModule ?? undefined; |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null coalescing to undefined is unnecessary. Since resolvedModule can only be LZ4Module | null | undefined, and line 34 ensures it's never undefined after first call, this could simply return resolvedModule || undefined or even just check for null. However, the cleanest approach is to return resolvedModule === null ? undefined : resolvedModule to make the null-to-undefined conversion explicit.
| return resolvedModule ?? undefined; | |
| return resolvedModule === null ? undefined : resolvedModule; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. @ikkala can you address this?
|
Thanks for the contribution! |
samikshya-db
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this fix, Ikkala.
+1 to @shivam2680 's comments
Earlier we had fixed LZ4 module loading for azure functions in #298 - this handled cases when lz4 wasn't available. But this lazy loading is a neat addition to it.
This changes the utlity module to return a function instead of the module directly. This way, caller can control when the module is actually tried to be resolved.
When lz4 disabled, we shouldn't get any error messages / warnings to console.