-
Notifications
You must be signed in to change notification settings - Fork 88
add lazy loaders for .cjs file #962
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
Conversation
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 implements lazy loading for CJS modules to avoid eagerly loading all ABIs when requiring the contracts package. The changes introduce a lazy loading mechanism using property getters and mark the package as side-effect free for better bundler optimization.
- Replaces direct
require()calls with lazy getters that defer loading until accessed - Adds a
__defineLazyhelper function to create property getters for module exports - Marks the package as having no side effects in package.json
Reviewed Changes
Copilot reviewed 2 out of 7 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/contracts/src/sync.ts | Implements lazy loading mechanism with __defineLazy helper and updates module export generation |
| packages/contracts/package.json | Adds sideEffects: false to enable better tree-shaking optimization |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| */ | ||
| // Define lazy getters so requiring the CJS root does not eagerly load all ABIs | ||
| function __defineLazy(target, key, getter) { |
Copilot
AI
Oct 17, 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 function parameters lack type annotations. In a TypeScript file, parameters should be typed for better type safety and IDE support.
| function __defineLazy(target, key, getter) { | |
| function __defineLazy(target: object, key: string | symbol, getter: () => any): void { |
| Object.defineProperty(target, key, { enumerable: true, configurable: true, get: getter }); | ||
| } | ||
| const __exports = {}; |
Copilot
AI
Oct 17, 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 __exports object lacks type annotation. Consider typing it as Record<string, any> or a more specific interface to maintain type safety.
| const __exports = {}; | |
| const __exports: Record<string, any> = {}; |
Ansonhkg
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.
I'm not sure this will work, this only approves the CJS lazy getters do their job at run time, sadly not when it bundles... If the bundler starts from CommonJS root, it will still drag in every network..
I think you're right. I made a little test repo here: https://github.com/LIT-Protocol/contract-shrinkage-test it seems like it worked for ESM, which I think is due to adding I'm not sure how to fix this in the CJS version tbh. |
The issue I think is actually from this mapper file: import { datilDev, datilTest, datil } from '@lit-protocol/contracts';This line resolves to the CJS root, so any build that runs through this file sees the whole contracts package and bundles every network blob. Since we already expose each contract bundle (in "exports": {
".": {
"import": "./dist/index.js",
"require": "./dist/index.cjs",
"types": "./dist/index.d.ts"
},
"./prod/datil": {
"import": "./dist/prod/datil.js",
"require": "./dist/prod/datil.cjs",
"types": "./dist/prod/datil.d.ts"
},
...
}I THINK, in the mapper we can do: import { datil } from '@lit-protocol/contracts/prod/datil';
import { datilDev } from '@lit-protocol/contracts/prod/datil-dev';
import { datilTest } from '@lit-protocol/contracts/prod/datil-test.';That keeps the existing mapper pattern, but allows bundlers to include just the network a consumer uses. I think the fix might be simplier than we thought? need to test though. |
|
Closing this in favour of this #963 |
I don't know how this actually works, I just asked cursor to fix the problem of including all the ABIs. we should prob test that this actually fixes the problem.
Anson:
WHAT
sideEffects: falseto package metadata so bundlers can tree-shake unused contract outputsExample
TEST
Snapshots are released here, but they are NOT tested yet. (Note:
@lit-protocol/contractsis backward-compatible, so we can use the same version for Datil, V7)