Build: connect plugins support esm #16729
Conversation
karliatto
force-pushed
the
build/connect-plugins-support-esm
branch
from
59214b8 to
71f5ac4
Compare
WalkthroughThe pull request introduces comprehensive changes to support ESM (ECMAScript Module) imports across multiple packages. The modifications include creating new ESM-specific build configurations, updating Assessment against linked issues
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| "exports": { | ||
| "node": "./lib/index.js", | ||
| "default": "./libESM/index.js" | ||
| }, |
There was a problem hiding this comment.
This is following the approach describes here https://webpack.js.org/guides/package-exports/#providing-commonjs-and-esm-version-stateless
Also about exports: https://nodejs.org/api/packages.html#package-entry-points
karliatto
force-pushed
the
build/connect-plugins-support-esm
branch
from
71f5ac4 to
eca775a
Compare
karliatto
added
the
no-project
karliatto
force-pushed
the
build/connect-plugins-support-esm
branch
2 times, most recently
from
b579921 to
cbab16e
Compare
karliatto
changed the title
karliatto
requested review from
mroz22,
martykan and
Nodonisko
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/replace-imports.sh (1)
40-45: Consider adding input validation.The script could benefit from validating the directory argument and checking if it exists.
Add this validation after setting variables:
else LIB_TYPE="lib" fi + +# Validate directory argument +if [ -z "$1" ]; then + echo "Error: Directory argument is required" + exit 1 +fi + +if [ ! -d "$1" ]; then + echo "Error: Directory '$1' does not exist" + exit 1 +fiAlso applies to: 48-53
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.easignore(1 hunks).gitignore(1 hunks).nxignore(1 hunks).prettierignore(1 hunks)packages/connect-plugin-ethereum/package.json(2 hunks)packages/connect-plugin-ethereum/tsconfig.libESM.json(1 hunks)packages/connect-plugin-stellar/package.json(2 hunks)packages/connect-plugin-stellar/tsconfig.libESM.json(1 hunks)packages/utils/package.json(1 hunks)packages/utils/tsconfig.libESM.json(1 hunks)scripts/replace-imports.sh(1 hunks)tsconfig.base.json(1 hunks)tsconfig.lib.json(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- .easignore
- tsconfig.base.json
- packages/connect-plugin-stellar/tsconfig.libESM.json
- packages/utils/tsconfig.libESM.json
- packages/connect-plugin-ethereum/tsconfig.libESM.json
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: e2e-test-suite-web (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: e2e-test-suite-web (@group=other, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=metadata, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=settings, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=device-management, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=suite, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group_wallet, trezor-user-env-unix bitcoin-regtest, 1)
- GitHub Check: e2e-test-suite-web (@group_other, trezor-user-env-unix, 1)
- GitHub Check: e2e-test-suite-web (@group_passphrase, trezor-user-env-unix, 1)
- GitHub Check: e2e-test-suite-web (@group_device-management, trezor-user-env-unix, 1)
- GitHub Check: e2e-test-suite-web (@group_suite, trezor-user-env-unix, 1)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
🔇 Additional comments (13)
.nxignore (1)
58-58: LGTM! Consistent with ESM build setup.The addition of
libESMto .nxignore aligns with the ESM support implementation and matches similar changes in other ignore files.tsconfig.lib.json (1)
18-18: LGTM! Appropriate TypeScript configuration for dual-format builds.The exclusion of libESM directory is correct for the CommonJS build configuration, ensuring clean separation between CJS and ESM builds.
packages/connect-plugin-ethereum/package.json (2)
29-32: LGTM! Well-structured exports configuration.The exports field correctly implements conditional exports, providing both CommonJS and ESM versions while maintaining backward compatibility.
43-45: Verify the replace-imports script handles ESM paths correctly.The build scripts look good, but we should ensure the replace-imports.sh script properly handles the new libESM parameter.
✅ Verification successful
The replace-imports script correctly handles ESM paths
The script has explicit support for ESM paths through its second parameter and properly transforms imports using different patterns for ESM (
@trezor/pkg/libESM) and CJS (@trezor/pkg/lib). The package.json is correctly configured to use this functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if replace-imports.sh handles ESM paths correctly cat ../../scripts/replace-imports.shLength of output: 103
Script:
#!/bin/bash # Find replace-imports.sh file in the repository fd "replace-imports.sh"Length of output: 50
Script:
#!/bin/bash # Examine the replace-imports script contents cat scripts/replace-imports.shLength of output: 2083
packages/utils/package.json (2)
23-24: LGTM! Good package optimization.The files array is well configured:
- Excludes map files to optimize package size
- Includes libESM for ESM support
29-31: LGTM! Consistent build configuration.The build scripts maintain consistency with other packages in the monorepo, properly separating CJS and ESM builds.
packages/connect-plugin-stellar/package.json (3)
25-28: LGTM! Exports field follows Node.js dual package hazard recommendations.The exports field correctly defines separate entry points for Node.js (CJS) and ESM environments, following best practices for dual package support.
30-31: LGTM! Files array includes both build outputs.Including both
lib/andlibESMensures that both CJS and ESM builds are published.
48-50: LGTM! Build scripts properly handle both CJS and ESM builds.The build process is correctly split into CJS and ESM builds, maintaining backward compatibility while adding ESM support.
.gitignore (1)
52-52: LGTM! Build output directory properly ignored.The
libESMdirectory is correctly added to .gitignore alongside other build outputs..prettierignore (1)
55-55: LGTM! Build output directory properly excluded from formatting.The
libESMdirectory is correctly added to .prettierignore alongside other build outputs.scripts/replace-imports.sh (2)
5-19: LGTM! Clear and comprehensive usage documentation.The added documentation clearly explains the script's usage, arguments, and provides helpful examples.
20-32: LGTM! Clean implementation of library type handling.The script properly handles both CJS and ESM paths through a configurable library type parameter.
karliatto
force-pushed
the
build/connect-plugins-support-esm
branch
from
cbab16e to
5defd33
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/utils/package.json (1)
23-24: Consider excluding source maps for libESM.The
!**/*.mapexclusion is added beforelibESM, which means it only applies to files before that entry. To ensure source maps are also excluded from the ESM build, consider moving the exclusion afterlibESM."files": [ "lib/", - "!**/*.map", - "libESM" + "libESM", + "!**/*.map" ],packages/connect-plugin-stellar/package.json (1)
30-31: Consider excluding source maps consistently.Similar to the utils package, consider adding the source maps exclusion to ensure consistency across packages.
"files": [ "lib/", - "libESM" + "libESM", + "!**/*.map" ],packages/connect-plugin-ethereum/package.json (1)
26-27: Add trailing slash for consistency.The
filesarray has inconsistent trailing slashes. Add a trailing slash tolibESMto match the format oflib/."files": [ "lib/", - "libESM" + "libESM/" ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.easignore(1 hunks).gitignore(1 hunks).nxignore(1 hunks).prettierignore(1 hunks)packages/connect-plugin-ethereum/package.json(2 hunks)packages/connect-plugin-ethereum/tsconfig.libESM.json(1 hunks)packages/connect-plugin-stellar/package.json(2 hunks)packages/connect-plugin-stellar/tsconfig.libESM.json(1 hunks)packages/utils/package.json(1 hunks)packages/utils/tsconfig.libESM.json(1 hunks)scripts/replace-imports.sh(1 hunks)tsconfig.base.json(1 hunks)tsconfig.lib.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/utils/tsconfig.libESM.json
- tsconfig.base.json
- packages/connect-plugin-ethereum/tsconfig.libESM.json
- tsconfig.lib.json
- .gitignore
- .easignore
- packages/connect-plugin-stellar/tsconfig.libESM.json
- .prettierignore
- .nxignore
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-deploy
- GitHub Check: transport-e2e-test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
🔇 Additional comments (9)
packages/utils/package.json (1)
29-31: LGTM! Build scripts properly handle both CJS and ESM formats.The build process is well-structured:
- Cleans output directories
- Builds using format-specific tsconfig
- Processes imports appropriately
packages/connect-plugin-stellar/package.json (2)
25-28: LGTM! Export configuration properly handles dual package hazard.The exports field correctly defines separate entry points for Node.js and ESM consumers while maintaining backward compatibility through publishConfig.
48-50: LGTM! Build scripts match the utils package structure.The build process follows the same well-structured approach as the utils package.
scripts/replace-imports.sh (4)
5-19: LGTM! Well-documented usage instructions.The documentation clearly explains the script's purpose, arguments, and provides helpful examples.
20-25: LGTM! Proper argument handling with sensible defaults.The script correctly handles the optional lib-type argument with a default value of "lib".
27-32: LGTM! Dynamic regex properly handles both formats.The regex is correctly adjusted based on the lib-type argument to handle both CJS and ESM paths.
Line range hint
1-53: Fix undefined LOG_FILE variable.The script uses
LOG_FILEfor logging changed files, but this variable is not defined.Add this line after the shebang:
#!/usr/bin/env bash +LOG_FILE="replace-imports.log" set -euxo pipefailpackages/connect-plugin-ethereum/package.json (2)
29-32: LGTM! Exports field follows recommended pattern.The
exportsfield correctly implements the dual package pattern, providing both CommonJS and ESM entry points. This aligns with webpack's documentation on package exports.Let's verify the TypeScript configurations referenced in the build scripts:
✅ Verification successful
Dual package pattern implementation verified successfully
The package correctly implements the dual package pattern with proper TypeScript configurations for both CommonJS (tsconfig.lib.json) and ESM (tsconfig.libESM.json) builds. This pattern is consistently implemented across multiple packages in the repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify TypeScript configurations for both CJS and ESM builds # Check if both tsconfig files exist and their module settings echo "Checking TypeScript configurations..." for config in tsconfig.lib.json tsconfig.libESM.json; do if [ -f "$config" ]; then echo "Found $config:" cat "$config" | grep -A 5 '"compilerOptions":' else echo "Missing $config" fi doneLength of output: 590
Script:
#!/bin/bash # Check build configuration and output files echo "=== Package.json Build Scripts ===" cat package.json | jq '.scripts' echo -e "\n=== Checking Output Directories ===" ls -la lib/ libESM/ 2>/dev/null || echo "Output directories not found" echo -e "\n=== Looking for Alternative TypeScript Configs ===" fd -e json -e jsonc . | grep -i "tsconfig"Length of output: 11845
43-45: Verify the replace-imports script behavior.The build scripts use a
replace-imports.shscript with different parameters for CJS and ESM builds. Let's understand its purpose and verify its implementation.✅ Verification successful
The replace-imports script implementation is correct and robust
The script properly handles both CJS and ESM builds by replacing
@trezor/*/srcimports with the appropriate lib paths. It includes error handling, cross-platform support, and verification steps to ensure all replacements are successful.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the replace-imports script implementation echo "Checking replace-imports.sh script:" cat ../../scripts/replace-imports.sh echo -e "\nChecking if the script handles both CJS and ESM cases:" ls -la ../../scripts/replace-imports.shLength of output: 431
Script:
#!/bin/bash # Search for replace-imports.sh in the repository echo "Searching for replace-imports.sh:" fd "replace-imports.sh" # Try searching in common script locations echo -e "\nChecking common script locations:" ls -la ./scripts/replace-imports.sh 2>/dev/null || echo "Not in ./scripts/" ls -la ./tools/scripts/replace-imports.sh 2>/dev/null || echo "Not in ./tools/scripts/"Length of output: 427
Script:
#!/bin/bash # Examine the replace-imports script contents echo "Content of replace-imports.sh:" cat ./scripts/replace-imports.sh # Check for related configuration files echo -e "\nChecking for related configuration files:" fd -e json "tsconfig" --max-depth 2Length of output: 2036
| "extends": "./tsconfig.lib.json", | ||
| "compilerOptions": { | ||
| "outDir": "libESM", | ||
| "module": "ESNext" |
There was a problem hiding this comment.
I am thinking that it might be good time to also target to esnext too, because there is no reason we should transpile to older es version
There was a problem hiding this comment.
karliatto
force-pushed
the
build/connect-plugins-support-esm
branch
2 times, most recently
from
699cb7e to
10fd34f
Compare
karliatto
force-pushed
the
build/connect-plugins-support-esm
branch
from
10fd34f to
75aa117
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/connect-plugin-stellar/package.json (1)
30-32: Add trailing slash to libESM for consistency.For consistency with the
lib/entry, consider adding a trailing slash to thelibESMentry."files": [ "lib/", - "libESM", + "libESM/", "!**/*.map" ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/connect-plugin-ethereum/package.json(2 hunks)packages/connect-plugin-ethereum/tsconfig.libESM.json(1 hunks)packages/connect-plugin-stellar/package.json(2 hunks)packages/connect-plugin-stellar/tsconfig.libESM.json(1 hunks)packages/utils/package.json(1 hunks)packages/utils/tsconfig.libESM.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/utils/tsconfig.libESM.json
- packages/connect-plugin-ethereum/tsconfig.libESM.json
- packages/connect-plugin-ethereum/package.json
- packages/connect-plugin-stellar/tsconfig.libESM.json
- packages/utils/package.json
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (2)
packages/connect-plugin-stellar/package.json (2)
25-28: LGTM! Well-structured exports configuration.The dual entry points for Node.js and ESM are correctly configured, providing clear access patterns for both module systems.
49-51: LGTM! Well-structured build scripts.The build process is correctly split to handle both CommonJS and ESM formats, with appropriate cleanup and import replacement for each format.
Description
Add esm support to connect plugins.
"exports"in package.json to define the entry points of the libraries. While keeping the previouspublishConfigfor bundlers that do not supportexports. Usingexportsgive us control over the entry points that are accessible they the developers using this packages, making it more clear in the future if we change somehting in source we can be safe since they only will use whatever entrypoints we define there.libESMthat will be publish to NPM.Related Issue
Resolve #16245