Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't import some modules #37

Closed
pimterry opened this issue Dec 13, 2023 · 6 comments · Fixed by #38
Closed

Can't import some modules #37

pimterry opened this issue Dec 13, 2023 · 6 comments · Fixed by #38
Assignees

Comments

@pimterry
Copy link
Collaborator

One example: node-datachannel.

Create a new folder, and run:

$ npm install import-sync node-datachannel
$ node
> importSync = require('import-sync')
> importSync('node-datachannel')
Uncaught Error: 
        Failed to import from:
            node-datachannel.
        Options:
            {}
        Require error message:
            /tmp/tmp.cGqoObetIA/node_modules/import-sync/dist/cjs/import.js:1
Error: Cannot find module 'node-datachannel'
Require stack:
- /tmp/tmp.cGqoObetIA/node_modules/import-sync/dist/cjs/import.js
- /tmp/tmp.cGqoObetIA/node_modules/import-sync/dist/cjs/index.js
- <repl>
    at esmImport (/tmp/tmp.cGqoObetIA/node_modules/import-sync/dist/cjs/import.js:47:16)
    at importSync (/tmp/tmp.cGqoObetIA/node_modules/import-sync/dist/cjs/import.js:72:28)
    at REPL4:1
    at Script.runInThisContext (node:vm:122:12)
    at REPLServer.defaultEval (node:repl:594:29)
    
    at esmImport (/tmp/tmp.cGqoObetIA/node_modules/import-sync/dist/cjs/import.js:50:15)
    at importSync (/tmp/tmp.cGqoObetIA/node_modules/import-sync/dist/cjs/import.js:72:28)

> await import('node-datachannel')
[Module: null prototype] {
  Audio: [Function: Audio],
  DataChannel: [Function: DataChannel],
  ... // I.e. it imports with ESM just fine
}

Happens with Node 20.8.0 and 21.4.0 (latest). Any idea what's going on here? Node-fetch does indeed import as expected, but this module isn't even found for some reason.

@nktnet1
Copy link
Owner

nktnet1 commented Dec 14, 2023

Hi @pimterry,

I briefly looked into node-datachannel and there are two issues that I can see.

1. Missing Main Field

node-datachannel's package.json is missing a "main" field

This can be fixed by adding

"main": "./lib/index.js",

at the top level.

2. Redefining Require

The variable "require" is being redefined in lib/index.js at lines 3-5:

While this is fine in ESM ("require" is replaced with "import"), if you are trying to use this module into your CJS project, you will get the SyntaxError:

Identifier 'require' has already been declared

This can be fixed by simply calling "require" something else, e.g. in node-datachannel's lib/index.js, lines 3-5:

const requireDifferentName = createRequire(import.meta.url);

const nodeDataChannel = requireDifferentName('../build/Release/node_datachannel.node');

Concluding Remarks

I currently don't see an easy and robust way of fixing it in import-sync, so I suggest making an issue about your usage on node-datachannel. However, their package appears to be intended for ESM-use only, so I'm not sure about the prospect of them making this niche fix for CJS.

Hacky Workaround

If you still wish to use node-datachannel and import-sync in your CJS project without waiting for a fix, you could write a setup shell script to make these changes directly to your node_modules/node-datachannel:

#!/bin/sh
# fix-node-datachannel.sh

nodeDatachannelPackagePath='./node_modules/node-datachannel/package.json'
nodeDatachannelIndexPath='./node_modules/node-datachannel/lib/index.js'

# Append "main" to node-datachannel's package.json
tempFile="$(mktemp)"
jq '.main = "./lib/index.js"' package.json > "$tempFile"
mv "$tempFile" "$nodeDatachannelPackagePath"

# Change "require" to "requireDifferentName" in node-datachannel's lib/index.js
sed -i 's/\brequire\b/requireDifferentName/g' "$nodeDatachannelIndexPath"

which will work as expected - example from CLI:

$  bash fix-node-datachannel.sh
$  node                         
Welcome to Node.js v18.18.2.
Type ".help" for more information.
> const importSync = require('import-sync');
> importSync('node-datachannel');
Proxy [
  Proxy [
    {
      initLogger: [Function (anonymous)],
      cleanup: [Function (anonymous)],
      preload: [Function (anonymous)],
      setSctpSettings: [Function (anonymous)],
      RtcpReceivingSession: [Function: RtcpReceivingSession],
      Track: [Function: Track],
      Video: [Function: Video],
      Audio: [Function: Audio],
      DataChannel: [Function: DataChannel],
      PeerConnection: [Function: PeerConnection],
      DataChannelStream: [class DataChannelStream extends Duplex],
      default: [Object]
    },
    {}
  ],
  {}
]

You can also look into potentially adding a postinstall script in your package.json to execute this shell script.

@pimterry
Copy link
Collaborator Author

Great research, thanks for looking into this @nktnet1! That's really helpful.

node-datachannel's package.json is missing a "main" field

That shouldn't be required though, as it does have an exports map which maps . (the root package export) to the correct file.

This resolution seems to work fine for CJS normally (require('node-datachannel') fails, but correctly reports the lib/index.js file as the problem, so it's clearly resolving it OK).

Any idea why that's not working here?

The latter const require issue is an easier fix, or could plausibly be something fixable automatically within ESM during processing, as part of the ESM->CJS transformation, but I'm not sure I understand why the main package.json field is required here, and I'm sure there will be plenty of other packages with that same issue.

@nktnet1
Copy link
Owner

nktnet1 commented Dec 14, 2023

Thanks @pimterry 😄. Regarding

node-datachannel's package.json is missing a "main" field

That shouldn't be required though, as it does have an exports map which maps . (the root package export) to the correct file.

I'm not too sure on this point and will some time looking into it in the next few days.

For reference, below is npm's package.json documentation for the main field:

The main field is a module ID that is the primary entry point to your program. That is, if your package is named foo, and a user installs it, and then does require("foo"), then your main module's exports object will be returned.

This should be a module relative to the root of your package folder.

For most modules, it makes the most sense to have a main script and often not much else.

If main is not set, it defaults to index.js in the package's root folder.

As for the "exports" property, I read up a bit on it in TypeScript's documentation:

as well as NodeJS's package-entry-points

But haven't quite pin-point the issue. One interesting thing I did discover though is that the the first issue can be fixed without adding the "main" field if you add a /lib after node-datachannel, i.e:

const importSync = require('import-sync');

console.log(importSync('node-datachannel/lib'));

Update 1

Found a potentially related issue:

@pimterry
Copy link
Collaborator Author

Regarding const require, it looks like this is quite a common pattern - https://github.com/search?q=%22const+require+%3D+createRequire%22&type=code shows 14k examples of the same thing on GitHub 😬. That's going to cause problems. I think this is what you need to do to do a synchronous require within ESM, and const require = ... is the standard example for this (straight from the node docs so that will break lots of things.

Fortunately, I think I've found a way to fix this within the esm package itself! I'll look into that further now and set up a little demo to test that.

Would you be open to switching to an esm fork to use that, if this works? The original package seems now unmaintained for years. Looks like https://github.com/wallabyjs/esm is actively maintained (as part of wallaby.js) and has done some useful updates in the meantime (updating Acorn, so it can parse modern syntax like ??) so that might be a good base.

I think we could set up a fork based on that (I'm assuming the wallaby team aren't interested in these contributions, but who knows) with changes to fix const require and exports map resolution. With those in place, this package would appear be a rock solid solution to using ESM from CJS, which would be really really useful. I'm here looking for exactly that - I'm trying to migrate a bunch of projects to ESM, and I desperately need a good option like this to be able to do so incrementally (without having to do a big-bang conversion to ESM at the top level before I can even start).

I think there's likely to be a widespread need for this as more projects start to try to migrate. Let me know if this is more hassle than you were looking for for this project though - I'm happy to fork and hack together my own solution on top instead, if that's easier.

@pimterry
Copy link
Collaborator Author

All done! I have two branches with esm package fixes here:

Neither is perfect, I can't claim to deeply understand all of esm, but they work and they both only affect cases that would otherwise definitely fail (missing modules, and top-level redeclarations of require).

With those included, using the result with this package I can sync-import node-datachannel no problem at all, and every other ESM-only package I've tried (like node-fetch) all still works as normal. So far 100% success rate 💪

Are you open to integrating this?

@nktnet1
Copy link
Owner

nktnet1 commented Dec 15, 2023

Wow, this is amazing @pimterry 🎉!

I've been digging into esm's codebase myself and was able to make some progress, but couldn't get it to work.


Would you be open to switching to an esm fork to use that, if this works

Yep, that's something I've had in mind for a while, just that import-sync has been able to find a workaround with a few hacks. Now's probably a good time for change though.

Are you open to integrating this?

Of course :) - would you like to publish @httptoolkit/esm to GitHub or npm? I will look into the wallaby base and see if there are conflicts/duplications with import-sync and fix those up.

@nktnet1 nktnet1 self-assigned this Mar 13, 2024
nktnet1 added a commit that referenced this issue Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants