version v10.2.1 breaks CJS

[boneskull]

Describe the Bug

After 3ea4ec5, the package is not resolvable via CJS.

Steps to Reproduce

1. Build project
2. Execute node -e "require('.')" from project root
3. Error:

   node:internal/modules/cjs/loader:1147
   throw err;
   ^
   
   Error: Cannot find module '.'
   Require stack:
   - /Users/boneskull/projects/nikku/didi/[eval]
     at Module._resolveFilename (node:internal/modules/cjs/loader:1144:15)
     at Module._load (node:internal/modules/cjs/loader:985:27)
     at Module.require (node:internal/modules/cjs/loader:1235:19)
     at require (node:internal/modules/helpers:176:18)
     at [eval]:1:1
     at runScriptInThisContext (node:internal/vm:144:10)
     at node:internal/process/execution:109:14
     at [eval]-wrapper:6:24
     at runScript (node:internal/process/execution:92:62)
     at evalScript (node:internal/process/execution:123:10) {
   code: 'MODULE_NOT_FOUND',
   requireStack: [ '/Users/boneskull/projects/nikku/didi/[eval]' ]
   }
   

Expected Behavior

The command above should exit with a non-zero exit code.

Environment

• Host (Browser/Node version), if applicable: [e.g. MS Edge 18, Chrome 69, Node 10 LTS] Node.js v20.10.0
• OS: [e.g. Windows 7] macOS
• Library version: [e.g. 2.0.0] latest

[nikku]

@boneskull thanks for the report.

How is this a valid usage of the project? Why would you expect to require('.') within diagram-js?

[boneskull]

err, that's just the easiest way to reproduce. use require('didi') somewhere.

[nikku]

You mean this? It works on my machine, so please provide concrete steps to reproduce:

~/camunda/projects/bpmn.io/.other/foo
> npm install didi

up to date, audited 3 packages in 512ms

found 0 vulnerabilities

~/camunda/projects/bpmn.io/.other/foo
> npm 

~/camunda/projects/bpmn.io/.other/foo
> cat package.json 
{
  "dependencies": {
    "didi": "^10.2.1"
  }
}

~/camunda/projects/bpmn.io/.other/foo
> node -e "console.log(require('didi'))"
{
  Injector: [Function: Injector],
  annotate: [Function: annotate],
  parseAnnotations: [Function: parseAnnotations]
}

~/camunda/projects/bpmn.io/.other/foo
> node --version
v20.11.1

[boneskull]

Yeah, this is weird. You're right--that works. I don't understand why require('.') doesn't, though. What I was ultimately trying to fix was the error that I get from TS, and I got sidetracked:

/path/to/some-file.ts:1:24 - error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("didi")' call instead.
  To convert this file to an ECMAScript module, change its file extension to '.mts', or add the field `"type": "module"` to '/path/to/package.json'.

1 import {Injector} from 'didi';
                         ~~~~~~

It appears that might be fixable as of TS v5.3 via import attributes (alas I am still on v5.2), but it is also fixable by the lib author using something like tshy. I am not sure the levers tshy pulls to avoid the above error, but it might be worth looking into if you intend to ship a dual module. I am doing this in snapshot-fs if you wish to see a relatively straightforward example.

[boneskull]

That said, if you put main back in, it will work as expected when required as a relative path:

{
  "main": "./dist/index.cjs"
}

Not sure your reasons for removing it.

[nikku]

@boneskull I've seen a bunch of bundlers having issues with main being a CJS module, not the ESM export. Hence I'm looking into migrating my libraries into ESM only and/or providing dual exports via the exports package.json field. At some point I plan to drop CJS support all together as we've seen many do in the larger eco-system.

I still do not understand under which circumstances you get typescript errors. You should not get them in my tests. But if "main": "..." fixes the situation for you (and it was in fact a breaking change to remove it), please provide me a setup I can reproduce this with and I'm happy to restore the past (if it does not break the future).

[nikku]

Consider to move your module resolution strategy in typescript to NodeNext. That should be aware of exports.

[nikku]

I found another case where removing main and module would break downstream libraries. I'll go ahead and revert that change. Better safe than sorry.

[boneskull]

Yeah, it breaks w/ older versions of node as well. AFAICT there's really no reason not to have a main.

The resolution strategy is using NodeNext, but it doesn't help with the TS errors. It's just an import {Injector} from 'didi' in a TS source file where TS is compiling to CJS. If I was compiling to ESM, I wouldn't have the error.

[boneskull]

Anyway, please let me know if you want to get to the bottom of it, and I can create a minimal repo to reproduce.