-
Notifications
You must be signed in to change notification settings - Fork 214
Fix/dual package hazard symbol hasinstance #990
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?
Fix/dual package hazard symbol hasinstance #990
Conversation
Add Symbol.hasInstance methods and unique markers to core classes (Address, Transaction, SignedTransaction, LogicSig, LogicSigAccount, AtomicTransactionComposer, ABIContract) to enable instanceof checks across module boundaries when CommonJS and ESM load separate SDK instances.
- Add tests for all classes with Symbol.hasInstance implementation - Test regular instanceof, custom Symbol.hasInstance, and cross-module simulation - Test edge cases with null, undefined, and wrong marker values
This means that any class that does NOT implement this fix may lead to unexpected when users try to use @lempira I think we'll probably want to also implement this for all the ABI types since they may get passed in to the SDK/algokit from users from different libs. |
- Add custom Symbol.hasInstance to all the ABITypes classes - Added tests
Updated to add the ABIType Classes |
'no-underscore-dangle': [ | ||
'error', | ||
{ | ||
allow: [ | ||
'_isAlgosdkAddress', | ||
'_isAlgosdkTransaction', | ||
'_isAlgosdkSignedTransaction', | ||
'_isAlgosdkLogicSig', | ||
'_isAlgosdkLogicSigAccount', | ||
'_isAlgosdkAtomicTransactionComposer', | ||
'_isAlgosdkABIContract', | ||
'_isAlgosdkABIUintType', | ||
'_isAlgosdkABIUfixedType', | ||
'_isAlgosdkABIAddressType', | ||
'_isAlgosdkABIBoolType', | ||
'_isAlgosdkABIByteType', | ||
'_isAlgosdkABIStringType', | ||
'_isAlgosdkABIArrayStaticType', | ||
'_isAlgosdkABIArrayDynamicType', | ||
'_isAlgosdkABITupleType', | ||
], | ||
}, | ||
], |
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 could just allow all leading _
here. Is that ok for this repo?
Fix dual package hazard with Symbol.hasInstance
Problem
When a package is loaded both as CommonJS and ES modules,
instanceof
checks can fail across module boundaries. This happens because the same class loaded through different module systems creates distinct class objects, causinginstanceof
to return false even for valid instances.@neilcampbell created a repo to reproduce this error. I tested this fix against that repo, and it worked.
Solution
Added
Symbol.hasInstance
static methods to core classes along with unique property markers to properly handle instance checks across module boundaries. This follows the Node.js recommended pattern for handling dual package hazard.For example:
This ensures that
instanceof
checks work correctly regardless of how the class is loaded (CommonJS or ESM).We investigate another solution of trying to update the webpack loader in this branch but that changed required an update to build process and thought this change would be easier.
Classes Updated
Core Classes:
ABI Type Classes:
Testing
Added comprehensive test suite in
tests/11.DualPackageHazard.ts
to verify cross-module instanceof behavior works correctly.Related