-
Notifications
You must be signed in to change notification settings - Fork 43
refactor [NET-1606]: Make utils package work without polyfilling
#3279
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
refactor [NET-1606]: Make utils package work without polyfilling
#3279
Conversation
This pull request updates the `Logger` utility to make its scoping more flexible and less dependent on NodeJS `module` objects.[^1] The changes allow the logger to be initialized with either a string or an object containing an `id`, making it easier to use in different contexts beyond just files or modules. It is backward compatible. ### Changes **Logger scope improvements:** * Changed the `Logger` constructor to accept a `scope` parameter, which can be either a string or an object with an `id` property, instead of a `NodeJS.Module`. (`packages/utils/src/Logger.ts`) * Updated the `Logger.createName` static method to handle the new `scope` type, extracting the identifier appropriately whether it's a string or an object. (`packages/utils/src/Logger.ts`) * Introduced the `Scope` type alias for clarity and type safety. (`packages/utils/src/Logger.ts`) ### Future steps * `module` is a Node-specific object and is not available in ES and browser making the logger hard to use in other environments without a polyfill. Next steps would be to stop initializing `Logger` with `module`, esp. in files targetting multiple environments – subject for a separate PR. [^1]: Extracted (with changes) from #3279.
047065f to
eb389ee
Compare
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 pull request refactors the @streamr/utils package to eliminate the need for polyfills by implementing platform-specific builds for Node.js and browser environments. The refactoring introduces a sophisticated dual-build system using Rollup, TypeScript project references, and module aliasing to ensure crypto and utility functions work natively in both environments without runtime polyfilling.
Key Changes:
- Introduced platform-specific implementations for cryptographic (getSubtle) and MD5 hashing (computeMd5) functions with separate Node.js and browser versions
- Implemented a Rollup-based build system that creates separate Node.js and browser bundles with proper module aliasing
- Restructured TypeScript configuration to use project references with separate configs for Node, browser, Jest, Karma, and Rollup builds
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.browser.json (root) | Adds root-level browser TypeScript configuration with DOM lib and bundler module resolution |
| packages/utils/tsconfig.rollup.json | Configures TypeScript compilation for the Rollup configuration file |
| packages/utils/tsconfig.node.json | Defines Node.js build configuration with path mappings to Node-specific implementations |
| packages/utils/tsconfig.karma.json | Adds browser-specific path mappings for Karma test environment |
| packages/utils/tsconfig.json | Restructures to use TypeScript project references for multi-target builds |
| packages/utils/tsconfig.jest.json | Adds Node-specific path mappings and module resolution for Jest tests |
| packages/utils/tsconfig.browser.json | Defines browser build configuration with path mappings to browser-specific implementations |
| packages/utils/test/composeAbortSignals.test.ts | Updates to use type-only import syntax |
| packages/utils/test/Metric.test.ts | Updates to use type-only import syntax |
| packages/utils/src/node/md5.ts | Implements Node.js MD5 hashing using native crypto module |
| packages/utils/src/node/crypto.ts | Implements Node.js SubtleCrypto access using native webcrypto API |
| packages/utils/src/keyToArrayIndex.ts | Refactors to use @md5 alias instead of direct crypto import |
| packages/utils/src/exports.ts | Adds buffer-shim import and updates getSubtle export to use @crypto alias |
| packages/utils/src/executeSafePromise.ts | Updates Logger initialization to use string literal instead of module global |
| packages/utils/src/crossPlatformCrypto.ts | Removes generic cross-platform crypto implementation (replaced by platform-specific versions) |
| packages/utils/src/browser/os.ts | Adds browser stub for Node.js os module |
| packages/utils/src/browser/md5.ts | Implements browser MD5 hashing using md5 library |
| packages/utils/src/browser/crypto.ts | Implements browser SubtleCrypto access using Web Crypto API |
| packages/utils/src/TheGraphClient.ts | Updates Logger initialization to use string literal instead of module global |
| packages/utils/src/SigningUtil.ts | Updates to import getSubtle from @crypto alias |
| packages/utils/src/Logger.ts | Adds env() helper function for safe process.env access in browser environments |
| packages/utils/rollup.config.mts | Creates Rollup configuration for building dual Node.js and browser bundles with platform-specific aliases |
| packages/utils/package.json | Updates exports structure for dual Node/browser builds, adds new dependencies, and includes postbuild Rollup step |
| packages/utils/karma.config.ts | Adds webpack alias configuration for browser-specific module resolution |
| packages/utils/jest.config.ts | Adds module name mapping for Jest tests (contains bug - maps to browser instead of node) |
| package-lock.json | Adds dependencies for Rollup, browser polyfills (buffer, md5, path-browserify, readable-stream), and updates package versions |
| Dockerfile.node | Adds @streamr/utils to bootstrap sequence as it's now a foundational dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e23df2c to
2d9d736
Compare
2d9d736 to
e5169c9
Compare
e5169c9 to
7281513
Compare
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
Copilot reviewed 25 out of 26 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@mondoreale I've opened a new pull request, #3318, to work on those changes. Once the pull request is ready, I'll request review from you. |
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
Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot, can u update PR desc? |
|
@mondoreale I've opened a new pull request, #3319, to work on those changes. Once the pull request is ready, I'll request review from you. |
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
Copilot reviewed 24 out of 25 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Copilot reviewed 24 out of 25 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Copilot reviewed 24 out of 25 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@mondoreale I've opened a new pull request, #3320, to work on those changes. Once the pull request is ready, I'll request review from you. |
92da48e to
c0d0a64
Compare
|
Too much noise here. I'll open another fresh PR. |
This pull request introduces significant improvements to the
@streamr/utilspackage, focusing on better cross-platform support (Node.js and browser), modular build outputs, and improved environment/configuration handling. The changes include a new build and packaging setup, environment variable abstraction, and refactoring of cryptographic utilities for compatibility across environments.Changes
Build and packaging modernization:
package.jsonto provide separate exports and type definitions for Node.js and browser, added build/type generation scripts, and included new dependencies for browser compatibility and bundling. [1] [2]rollup.config.mtsto generate environment-specific bundles and types, with proper aliasing for modules likeos,buffer, andpath.@streamr/utilspackage as part of the build process.Cross-platform and environment abstraction:
envabstraction, replacing directprocess.envusage throughout the codebase, and provided a browser shim for environment variables. [1] [2] [3] [4] [5] [6]osandbuffer, and updated test/build configs for aliasing. [1] [2] F8929faeL1, [3]Cryptography and hashing refactor:
Testing and module aliasing:
Build scripts and dependency updates: