Closed duanyytop closed 1 month ago
I will test if the following changes bring any issues:
clean
related commands are deleted
temp.ts
; Then build, the temp.ts
should be contained within the build dir; After that, remove the temp.ts
, and build again, the file should not be found in the build dirtsconfig.json
and tsconfig.build.json
config files are deletedpnpm install
; After the dependency installation, check if any issues are reported by the IDE from the src
and tests
directoriestempVar
variable from rgbpp-sdk/ckb
and build, import it in a random file in the examples, the IDE should prompt an error because the examples are now using the rgbpp@0.4.0
, which does not contain tempVar
rgbpp
as defined in the workspace; Therefore, pnpm
reused the local workspace version instead of downloading it from the internetrgbpp-sdk/ckb
in the rgbpp-sdk/service
lib’s source, the IDE should prompt an error because rgbpp-sdk/ckb
is not a dep of rgbpp-sdk/service
vitest.config.mts
in btc/service are deleted, and the config in ckb is moved to the repo-roottemp/Included.test.ts
file and run test, the file should be includeddist/Excluded.test.ts
file and run test, the file should be excludedpackage.json
calls npm scripts instead of direct calling fix commands of eslint/prettieA snapshot version is released: 0.0.0-snap-20240702110120
Will also test if the refactored libs can be imported/called normally in both ESM and CJS projects:
[x] CJS
rgbpp
to a CLI project, and it runs well in dev modeFailed to bundle the CLI to a binary executable file with esbuild
(no other modifications except updating rgbpp
) (fixed at 4f61ed8, 6096395 and 53fc8ee):
build/index.js:94948
var ECPair = (0, import_ecpair.default)(import_secp256k1.default);
^
TypeError: (0 , import_ecpair.default) is not a function
at build/index.js:94948:40
at embedderRunCjs (node:internal/util/embedding:37:10)
[x] ESM
Encountered a runtime error when running node xx.mjs
command in a native ESM project where its package.json
has "type": "module"
specified:
Error: Cannot find module '/Users/xx/projects/test-rgbpp-pure-esm/node_modules/@nervosnetwork/ckb-sdk-utils/lib/calculateTransactionFee' imported from /Users/xx/projects/test-rgbpp-pure-esm/node_modules/@rgbpp-sdk/ckb/dist/index.js
Did you mean to import "@nervosnetwork/ckb-sdk-utils/lib/calculateTransactionFee.js"?
at finalizeResolution (node:internal/modules/esm/resolve:264:11)
at moduleResolve (node:internal/modules/esm/resolve:924:10)
This seems to be an issue between Node, ESM, and TypeScript, where NodeNext requires relative/absolute imports to contain file extensions. However, all our imports in the source code were written without file extensions.
But TBH, package imports are the real cause of the error. During the tsup
bundling process, all our .ts
files were merged into a single .js
file, which means there's no imports needed, therefore they don't cause import-related issue. However, after the build, the package imports remained the same as before as external imports, and normally without file extensions.
To address the issue, we may consider adding .js
extensions to the non-index package imports. Additionally, the ESM packages should still be tested more deeply before we ship them. Maybe we should invite more app teams to test the functionality of the ESM packages. If their usage differs from ours, they might not encounter the same errors we did.
Here's a reproducible repo for the ESM tests: https://github.com/ShookLyngs/test-rgbpp-pure-esm. You can clone the repo and run a node command like node test-import/ckb.mjs
to reproduce the error.
(fixed at 4f61ed8, 6096395 and 53fc8ee)
The lint staged commands and changeset have been updated @ShookLyngs
The new commits 4f61ed8, 6096395 and 53fc8ee should have resolved the previous issues. If they occur again, we can update them in new comments to make the entire timeline clearer.
The repo for testing the compatibility of ESM/CJS has also been updated to refence a new snapshot version (0.0.0-20240705090030
): https://github.com/ShookLyngs/test-rgbpp-pure-esm/commit/789c114ba8fa71ffc368e0c49a904b97b88d0a61. You can run command make test
to test if the rgbpp
package can be imported and executed in both .cjs
and .mjs
environments.
moduleResolution
to NodeNext
?Previously, when testing rgbpp
in ESM, an error was reported indicating that subpath imports should always contain file extensions. For example, when importing a JS file, the .js
extension must be included in the import syntax:
import { xx } from './a'; // Invalid
import { xx } from './a.js'; // Valid
Ref to documentation of Node.js for more: https://nodejs.org/api/esm.html#mandatory-file-extensions
Since all our source files will be packed into a single file when bundling with tsup
, there should be no import { xx } from './a'
syntax. The only type of imports left after bundling are package imports for importing packages. To address the issue, all subpath package imports in the packages have been refactored to include the .js
file extension:
import { xx } from 'pkg/lib/some'; // From
import { xx } from 'pkg/lib/some.js'; // To
However, there is still an issue: our current tsconfig
settings do not enforce the inclusion of file extensions in imports, and both tsc
and tsup
do not handle file extension in the imports. Related discussions:
This leads to an incompatibility problem where, if in the future we add new package dependencies, there is nothing to help us check or validate if the new imports are handled correctly:
import { a } from 'old-pkg/lib/some.js'; // Handled correctly
import { b } from 'new-pkg/lib/other'; // Lacks ".js" extension in the path, but no warning
"moduleResolution": "NodeNext"
in tsconfig.json
. This requires all relative imports to contain file extensions, but we would need to refactor all our code to satisfy this requirement.eslint
to add a rule that requires package imports to include file extension: https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/extensions.md.Ideas and comments are welcomed.
The first solution makes sense to me and the reason can be found in the following comment https://github.com/ckb-cell/rgbpp-sdk/pull/246#discussion_r1670137658.
I have tested in JoyID, Next.js, and Node.js scripts with rgbpp-sdk
(0.0.0-snap-20240801024204) whose packages both support ESM and CommonJS, and they all worked well.
cc @ShookLyngs @Flouse
Main Changes
ts-node
to run examples and testsckb-sdk-js
to support ESM