refactor(rgbpp-sdk): Support ESM package

[duanyytop]

Main Changes

• Replace turbo with tsup
• Export CommonJS(cjs) and ECMAScript(esm) packages for rgbpp sdk
• Use tsx instead of ts-node to run examples and tests
• Upgrade ckb-sdk-js to support ESM
• Clean up the CKB library import packages

[ShookLyngs]

I will test if the following changes bring any issues:

• The clean related commands are deleted
  • 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
• The tsconfig.json and tsconfig.build.json config files are deleted
  • 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
  • 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
  • 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)
• The vitest.config.mts in btc/service are deleted, and the config in ckb is moved to the repo-root
  • Run tests for multiple times, and check if any timeout errors are shown in the results
  • Add a temp/Included.test.ts file and run test, the file should be included
  • Add a dist/Excluded.test.ts file and run test, the file should be excluded
• lint-staged in the package.json calls npm scripts instead of direct calling fix commands of eslint/prettie
  • Update a file to contain bad styling, commit it and then check if the styling issue is fixed by the lint-staged
  • 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:

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

• 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]

The lint staged commands and changeset have been updated @ShookLyngs

[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): ShookLyngs/test-rgbpp-pure-esm@789c114. You can run command make test to test if the rgbpp package can be imported and executed in both .cjs and .mjs environments.

[ShookLyngs]

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:

• Adding support for ESM references without a .js extension  nodejs/node#46006
• bug(esm): TypeScript is not an ECMAScript superset post-ES2015 microsoft/TypeScript#50501

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:

1. 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.
2. 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.
3. 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]

The first solution makes sense to me and the reason can be found in the following comment #246 (comment).

[duanyytop]

The TypeScript document recommends using "moduleResolution": "Node" instead of "moduleResolution": "Bundler" for writing a library.

In short, "moduleResolution": "bundler" is infectious, allowing code that only works in bundlers to be produced. Likewise, "moduleResolution": "nodenext" is only checking that the output works in Node.js, but in most cases, module code that works in Node.js will work in other runtimes and in bundlers.

See: https://www.typescriptlang.org/docs/handbook/modules/guides/choosing-compiler-options.html#considerations-for-bundling-libraries

https://www.typescriptlang.org/docs/handbook/modules/guides/choosing-compiler-options.html#im-writing-a-library

[duanyytop]

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.

According to the above comment, the current code has added .js extension for the relative imports, so we should use "moduleResolution": "NodeNext" instead of "moduleResolution": "Bundler".

[ShookLyngs]

My thoughts, at the beginning

According to the TS 5.0+ related documentation, both options are recommended for modern projects. However, after some tests, I think the Bundler option is somewhat more reasonable to go with. Though this is not final, so you can continue reading. Any feedback is welcome.

Firstly, I think we have 3 paths to go:

• Specify the packages as "type": "module", and specify "moduleResolution": "NodeNext"
• Keep the packages as "type": "commonjs", and specify "moduleResolution": "NodeNext"
• Keep the packages as "type": "commonjs", and specify "moduleResolution": "Bundler"

And I think the Bundler option is more reasonable:

1. We're in fact using tsup as the bundler, which transpiles/packs our source code into single CJS/ESM files.
2. The Bundler option is an easier option for migration as it prevents a huge refactor of our code.

Let's first look at the difference between NodeNext and Bundler.

Difference between NodeNext and Bundler

While the two options mostly behave the same in the ways we care, and both of them are recommended options for modern projects, yet they do have differences. Refer to this PR for more: microsoft/TypeScript#51669.

The NodeNext option requires all relative imports in ESM to include a file extension, meaning all the following import examples should be refactored to contain a file extension .js at the end of the path:

import { a } from './a';      // Invalid
import { a } from './a.js';   // Correct

import { b } from 'pkg/b';    // Invalid
import { b } from 'pkg/b.js'; // Correct

The Bundler option, however, doesn't treat file extensions as mandatory and will leave the problem to our bundler: tsup. While bundling, tsup packs all source code into a single JS file, which means in our bundled code, there are no local imports, only the package imports remain:

import { a } from './a';      // This line doesn't exist in the bundled code
import { b } from 'pkg/b';    // This line exists in the bundled code

So with the Bundler option, we only need to deal with the package imports:

import { b } from 'pkg/b';    // Invalid
import { b } from 'pkg/b.js'; // Correct

File extension are just promises

For more discussions: microsoft/TypeScript#49083

Another thing about the file extension being mandatory is that our bundlers (tsc and tsup) refuse to transpile the file extension of the imports from .ts to .js for us while bundling. That is, if we decide to add file extensions to the relative imports in our source code, even if we're importing a .ts file, we have to use .js like they're some sort of promises:

import { a } from './a.js';   // There's only an "a.ts" file in the directory

However, adding .js to package imports feels okay because they are usually just .js files:

import { b } from 'pkg/b.js'; // The "pkg/b.js" file actually exists

The costs of migrating to ESM-first packages

What if we just want to migrate to specifying the "type": "module" and "moduleResolution": "NodeNext" together, as they are the most recommended settings for ESM-first packages?

Here's a branch that has done most of the refactor work, where all relative imports are refactored to include a file extension, and "type": "module" is added to the package.json to specify explicitly that the package is an ESM-first library:

• Commit: fedb0f6
• Snapshot: 0.0.0-20240709190558
• Changes:
  1. Add file extension .js to all relative imports.
  2. Add "type": "module" to package.json to tag explicitly that this is an ESM-first package.
  3. Fix some type issues or weird incompatibilities found in the rgbpp-sdk/ckb lib to prevent bundling failures.
• Test results:
  • Tested in https://github.com/ShookLyngs/test-rgbpp-pure-esm in both ESM/CJS.
  • Tested in a CLI project to bundle it into a single executable application.

Use NodeNext without migration

While testing everything, I find that we could specify the NodeNext option as the module resolution and NOT set "type": "module" in the package.json, which means by default our packages are just treated as CJS. With this combination, the TSC actually reports no error about our relative imports, even if they have no file extension included.

But, the problems are:

1. I'm not sure if this is a valid thing to do, or if it is simply an unresolved bug. Any documentation about this?
2. If we're using this combination, what's the difference from using the Bundler option? As they both don't give warnings from the TSC and IDE, we still need to validate the packages after bundling them.

Conclusion

After everything, these are my thoughts about the combinations:

• Specify the packages as "type": "module", and specify "moduleResolution": "NodeNext"
  • We have to refactor most of the code, and the file extensions in the imports are not what they look like.
  • If we decide to accept the downsides, this is the most recommended choice by the community.
• Keep the packages as "type": "commonjs", and specify "moduleResolution": "NodeNext"
  • Behaves the same as Bundler, no file extension mandatory, but weirdly, I'm not sure if this combination is a bug of TSC or if it is simply a valid choice.
  • If we can find more documentation about this, maybe this method is also acceptable.
• Keep the packages as "type": "commonjs", and specify "moduleResolution": "Bundler"
  • The bundled code is not promised to work at runtime because the file extensions for package imports are not mandatory, therefore the packages are lacking a validation process to keep the them work smoothly.
  • If we treat tsup as a bundler, we can use this option.

[duanyytop]

Have you tried with the "module": "node16" and "moduleResolution": "nodenext" of the tsconfig.json?

See https://www.typescriptlang.org/docs/handbook/modules/guides/choosing-compiler-options.html#im-writing-a-library

• module: "node16". When a codebase is compatible with Node.js’s module system, it almost always works in bundlers as well. If you’re using a third-party emitter to emit ESM outputs, ensure that you set "type": "module" in your package.json so TypeScript checks your code as ESM, which uses a stricter module resolution algorithm in Node.js than CommonJS does. As an example, let’s look at what would happen if a library were to compile with "moduleResolution": "bundler":

export * from "./utils";

Assuming ./utils.ts (or ./utils/index.ts) exists, a bundler would be fine with this code, so "moduleResolution": "bundler" doesn’t complain. Compiled with "module": "esnext", the output JavaScript for this export statement will look exactly the same as the input. If that JavaScript were published to npm, it would be usable by projects that use a bundler, but it would cause an error when run in Node.js:

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '.../node_modules/dependency/utils' imported from .../node_modules/dependency/index.js
Did you mean to import ./utils.js?

[ShookLyngs]

Yes, I've tried it, a TS error will be thrown if we don't specify them in pairs (that's why I didn't mention it):

error TS5110: Option 'module' must be set to 'NodeNext' when option 'moduleResolution' is set to 'NodeNext'

You can refer to this tsconfig.json for example: fedb0f6#diff-339644dcceb3090377506d9dad91839e1b8916140ad6a467d703427ea83c6233.

[ShookLyngs]

I'm giving a like to the PR because I think the current implementation works by keeping "type": "commonjs" in the package.json, and specify "moduleResolution": "Bundler" in the tsconfig.json. Also, the implementation includes the least changes to the codebase.

[duanyytop]

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.

• JoyID: https://joyid-app-git-rgbpp-test-nervina.vercel.app/
• Next.js project: https://github.com/duanyytop/node-esm-test/tree/master/apps/next-rgbpp
• Node scripts: https://github.com/duanyytop/node-esm-test/blob/master/apps/scripts/lock-seal-onchain.mjs

cc @ShookLyngs @Flouse