nats-io / nats.ws

WebSocket NATS
Apache License 2.0
316 stars 27 forks source link

Get error using nats.ws and angular 17 #241

Closed fclmman closed 4 months ago

fclmman commented 4 months ago

Observed behavior

get error building project Could not resolve "crypto"

node_modules/nats.ws/esm/nats.js:4913:24:
  4913 │       crypto1 = require('crypto');
       ╵                         ~~~~~~~~

The package "crypto" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.

Expected behavior

project builds without error

Server and client version

"nats.ws": "^1.26.0", "@angular": "^17.3.0",

Host environment

No response

Steps to reproduce

create project with ng new, add nats.ws create connection and try to build

aricart commented 4 months ago

There's some bundler configuration that is happening here - the nats.ws/esm/nats.js doesn't have a single require there as shipped.

I believe your bundler is picking up nats.ws/cjs/nats.js which will have require this is only useful if you have a shim for node that does w3c sockets and will never ever work in the browser. Make sure you are not doing any require when importing the nats.ws library.

It should be import {connect} from "node_modules/nats.ws/esm/nats.js"

aricart commented 4 months ago

You may want to extract the "node_modules/nats.ws/esm/nats.js" file and reference it directly - depending on your bundler it may not correctly select between cjs and esm.

nickchomey commented 3 months ago

I get similar issues when I try to run nats.js through esbuild in order to minify and tree shake it.

It stems from the require('crypto') here https://github.com/nats-io/nkeys.js/blob/88d9d1c6479755ab4a215a7d5265b9d01990c57e/modules/esm/nacl.js#L1172C6-L1172C32

It seems that esbuild is trying to resolve this, but since I am building for the browser, it fails. If I build for nodejs, it succeeds, but that's not what I want.

If you wrap the require in a try.. catch, such as the following, it will work

} else if (typeof require !== 'undefined') {
    try {
        crypto1 = require('crypto');
        if (crypto1 && crypto1.randomBytes) {
            nacl.setPRNG(function(x, n) {
                var i, v = crypto1.randomBytes(n);
                for (i = 0; i < n; i++) x[i] = v[i];
                cleanup(v);
            });
        }
    } catch (e) {
        console.log('crypto module is not available in this environment');
    }
}

If you do a dynamic import, it works as well.

import('crypto')
.then(crypto => {
    if (crypto && crypto.randomBytes) {
        nacl.setPRNG(function(x, n) {
            var i, v = crypto.randomBytes(n);
            for (i = 0; i < n; i++) x[i] = v[i];
            cleanup(v);
        });
    }
})
.catch(err => {
    console.log('crypto module is not available in this environment');
});

I tried building with bun, and it seems to work regardless, but that's because it imports the entire node:crypto module into the bundle. Neither try..catch or dynamic import seems to affect this.

Would it be possible to add one of these solutions, so that at least esbuild (and whatever OP was using) will work?

Its a similar issue to what I reported here https://github.com/nats-io/nats.js/issues/608#issuecomment-1847334882

Side note - Deno bundle has been deprecated, and it doesnt have minify anyway. Perhaps something else (e.g. bun) could be used instead?

aricart commented 3 months ago

@nickchomey bundle is now an external api, it is still supported - with that said, I am in the middle of an extensive refactoring for the clients, which will split the lib into several - core, jetstream, kv, object store and services. Each will have an esm only option or a node option - the ESM only (via jsr) will do exactly what you want as it will only involve webapis.

nickchomey commented 3 months ago

Perfect! In the meantime I've forked the repo and applied my changes.

playerx commented 3 weeks ago

Hi guys, still having the same issue with Angular 18:

✘ [ERROR] Could not resolve "crypto"

    node_modules/nats.ws/esm/nats.js:4911:24:
      4911 │       crypto1 = require('crypto');

@nickchomey can you share the forked version please? Or maybe there is other solution?

@aricart I checked, but didn't find nats.ws on jsr, can you share if it's there? maybe with other name?

thanks 🙌🏻

aricart commented 3 weeks ago

The esm lib shouldn't have a require, as that is not available in browser, so all browser apps would fail. Shims for crypto should be available on npm...

playerx commented 3 weeks ago

@aricart Yes I agree, I think issue is that nats.ws loads crypto.

I've created a simple project to reproduce the issue: https://github.com/playerx/nats-issue

Steps I did:

  1. Create a new angular with ng new command
  2. Install and use nats.ws by running: npm i nats.ws

And npm run build fails with the error I mentioned.

If there is a way to avoid loading crypto please let me know.

playerx commented 3 weeks ago

Not sure if installing shim is the right way to go. I think the main problem is that it loads crypto when it shouldn't.

playerx commented 3 weeks ago

Workaround: Adding the following in package.json did the trick:

"browser": {
    "crypto": false
  }

However, I think for the long-term solution nats.ws shoudn't be loading crypto in browser.

aricart commented 3 weeks ago

Crypto is required for objectsstore, and works perfectly in browser...

https://developer.mozilla.org/en-US/docs/Web/API/Crypto

aricart commented 3 weeks ago

The 3.0 version of the clients breaks the lib into several. Crypto dependency will be on the objectstore module only Stay tuned