Cre8/issue1321

[cre8]

What issue is this PR fixing

fixes: #1321

The utils package is build with esbuild. However the current task fails since the build task in the package.json file with "build": "node esbuild.js && tsc --emitDeclarationOnly", will not exectute the tsc command. Even when putting the command in a dedicated job like "types": "tsc --emitDeclarationOnly", it fails. However calling it directly in the folder (and not via the package.json), it works.
To get all build work for now:

• go to the utils folder and run npm run build
• run also tsc --emitDeclarationOnly
• in the root folder run pnpm build to build all the other packages
• run pnpm test where everything will be passed

Update: when packing the package.json executable the task works (with chmod +x package.json)

What is being changed

It will publish the packages in ESM and commonjs so all developers are happy :)

Quality

Check all that apply:

• I want these changes to be integrated
• I successfully ran pnpm i, pnpm build, pnpm test, pnpm test:browser locally.
• I allow my PR to be updated by the reviewers (to speed up the review process).
• I added unit tests.
• I added integration tests.
• I did not add automated tests because _________, and I am aware that a PR without tests will likely get rejected.

Details

If applicable, add screen captures, error messages or stack traces to help explain your problem.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (165de35) 85.50% compared to head (59ee796) 85.50%.

Additional details and impacted files

@@           Coverage Diff           @@
##             next    #1322   +/-   ##
=======================================
  Coverage   85.50%   85.50%           
=======================================
  Files         170      170           
  Lines       18946    18946           
  Branches     2115     2115           
=======================================
  Hits        16200    16200           
  Misses       2746     2746           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mirceanis]

The build failures you were seeing were caused by the presence of the tsconfig.tsbuildinfo file. If you delete that between builds you should see the call to tsc succeed. This is also why the build works in the GitHub workflow.

I have some concerns about the CJS output referring to bundled dependencies by their pnpm path. I'm not sure if this would be an issue since I haven't tested the result of this PR.

That being said, how do you propose to test if this set of changes would improve things?

[cre8]

In case of testing: I am not sure if we can write tests for a CJS and a ESM case. But it would allow to use the libraires in frameworks like nestjs that are focussed on CJS (you can compile to ESM, but then some features are missing).

Even when in some years ESM dominates the backend market, we do not need to support both approaches (unless we see a big drawback with this approach)

[cre8]

One thing that is not covered in the current update is the type-check. This has to be done with tsc. I excluded it since this gave some errors according to other libraries.

So the pro argument of this is that we have CJS and ESM. The downside is more complexity since we are using TSC (for type checking and type generation) and es build (for converting to JS and bundling for CJS). The argument "compiles faster" is not really there, since even when using it in other dependencies you need to generate the types with tsc (I haven't measured the times tsc need for compiling + type generation and only type generation).

Writing these lines I am not 100% if this is a feature worth of trying. Maybe esbuild is good for applications where you don't care about available types, you just want a bundled pack of JS files...

[mirceanis]

I don't mean unit/integration testing necessarily. I mean, do you have a project that fails to build/work using the current ESM build where we could test the new build setup?

We can publish these changes under the @unstable distTag before merging them into next so there would be a npmjs ready artifact that can be used for testing.

[cre8]

I like the unstable option. I can setup a nestjs project with then openapi integration. There I always failed to use ESM

[cre8]

I updated all packages to esbuild and the build command passes. However I removed also the '.js' extensions for the import paths because I thought you do not need them (and there should be a solution for this). WIth the flag node --experimental-specifier-resolution=node ../cli/bin/veramo.js dev extract-api I was able to call it, but then I am getting this error:

packages/core extract-api: file:///C:/Users/mollik/Projects/veramo/packages/core-types/build/types/ICredentialStatusManager.js:1
packages/core extract-api: import { DIDDocument, DIDResolutionOptions, DIDResolutionResult } from "did-resolver";
packages/core extract-api:          ^^^^^^^^^^^
packages/core extract-api: SyntaxError: The requested module 'did-resolver' does not provide an export named 'DIDDocument'
packages/core extract-api:     at ModuleJob._instantiate (node:internal/modules/esm/module_job:124:21)
packages/core extract-api:     at async ModuleJob.run (node:internal/modules/esm/module_job:190:5)
packages/core extract-api: Node.js v18.17.0

Before reverting the commit with the '.js transformation', I just wanted to check if someone of you has an easy solution how we can deal with it (this explains the +200 file changes ;))

[cre8]

I continued the work in a new PR: #1328 since this one had to much files changed