refactor(rgbpp-sdk): Support ESM package

duanyytop commented 2 weeks ago

Main Changes

Replace turbo with tsup
Export CommonJS(cjs) and ECMAScript(esm) packages for rgbpp sdk
Use rgbpp version as a dependency for examples
Clean up the CKB library import packages

ShookLyngs commented 2 weeks ago

I will test if the following changes bring any issues:

[x] The clean related commands are deleted
- [x] Clean environment, then add a new file 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 dir
- [x] The tsconfig.json and tsconfig.build.json config files are deleted
- [x] Remove all dependencies from repo-root and the libs, and then run pnpm install; After the dependency installation, check if any issues are reported by the IDE from the src and tests directories
- [x] Export a tempVar 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
  - The IDE unexpectedly reports nothing
  - This is not an issue caused by the PR, it is because the examples are using the same version of rgbpp as defined in the workspace; Therefore, pnpm reused the local workspace version instead of downloading it from the internet
- [x] Import any function from rgbpp-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
  - The IDE unexpectedly reports nothing (fixed at 4f61ed8)
- [x] The vitest.config.mts in btc/service are deleted, and the config in ckb is moved to the repo-root
- [x] Run tests for multiple times, and check if any timeout errors are shown in the results
- [x] Add a temp/Included.test.ts file and run test, the file should be included
- [x] Add a dist/Excluded.test.ts file and run test, the file should be excluded
- [x] lint-staged in the package.json calls npm scripts instead of direct calling fix commands of eslint/prettie
- [x] Update a file to contain bad styling, commit it and then check if the styling issue is fixed by the lint-staged
- [x] Update two files to each contain bad styling, commit one of them, and then check whether the styling issue in the uncommitted file remains unfixed by the lint-staged
  - The uncommitted file is also fixed by the lint-staged (fixed at 87fad44)

A 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

Upgraded the snapshot version of rgbpp to a CLI project, and it runs well in dev mode

Failed 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)

duanyytop commented 1 week ago

The lint staged commands and changeset have been updated @ShookLyngs

ShookLyngs commented 1 week ago

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.

ShookLyngs commented 1 week ago

Discussion: Should we change the `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

Possible solutions:

Update "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.
Configure 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.
Add "bundle libs" and "execute bundled libs" as a form of integration testing to ensure packages can be executed in both CJS and ESM environments.

Ideas and comments are welcomed.

duanyytop commented 1 week ago

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.

ckb-cell / rgbpp-sdk