Add Browser-safe isSet & isMap implementations #98
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes the codebase browser-compatible by replacing Node.js-specific util/types imports with custom implementations that work in both browser and Node.js environments.
Changes:
- Added browser-safe
isSetandisMaputility functions toutils.ts - Updated imports in
custom-type.tsandcustom-kind.tsto use the new utility functions instead of Node.jsutil/types
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/typir/src/utils/utils.ts | Introduces browser-compatible isSet and isMap functions using instanceof checks with Object.prototype.toString fallbacks |
| packages/typir/src/kinds/custom/custom-type.ts | Replaces util/types import with new browser-safe utilities from utils.ts |
| packages/typir/src/kinds/custom/custom-kind.ts | Replaces util/types import with new browser-safe utilities from utils.ts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function isSet(value: unknown): value is Set<any> { | ||
| return value instanceof Set || Object.prototype.toString.call(value) === '[object Set]'; | ||
| } |
There was a problem hiding this comment.
The dual-check approach may be redundant in most scenarios. The instanceof check should handle both browser and Node.js contexts for native Set objects. The Object.prototype.toString.call() fallback is primarily useful for Sets from different JavaScript realms (e.g., iframes), but this scenario is uncommon. Consider whether the additional check is necessary for your use case, as instanceof Set alone is typically sufficient and more performant.
| export function isMap(value: unknown): value is Map<any, any> { | ||
| return value instanceof Map || Object.prototype.toString.call(value) === '[object Map]'; | ||
| } |
There was a problem hiding this comment.
The dual-check approach may be redundant in most scenarios. The instanceof check should handle both browser and Node.js contexts for native Map objects. The Object.prototype.toString.call() fallback is primarily useful for Maps from different JavaScript realms (e.g., iframes), but this scenario is uncommon. Consider whether the additional check is necessary for your use case, as instanceof Map alone is typically sufficient and more performant.
Fixes a small issue I ran into when bundling web-workers, the internal import of `util/types` is only available in a nodejs context. This adds in a pair of drop-in functions that are browser-safe, and should still work the same in a node context as they did before.
Fixes a small issue I ran into when bundling web-workers, the internal import of
util/typesis only available in a nodejs context. This adds in a pair of drop-in functions that are browser-safe, and should still work the same in a node context as they did before.