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

v1.9.0 stopped working with vitejs (rollup) #168

Closed
jonaskello opened this issue Sep 26, 2022 · 9 comments
Closed

v1.9.0 stopped working with vitejs (rollup) #168

jonaskello opened this issue Sep 26, 2022 · 9 comments

Comments

@jonaskello
Copy link

jonaskello commented Sep 26, 2022

We use vitejs to do production builds (vite build). I think vitejs in turn uses rollup under the hood.

When moving from nats.ws version 1.8.1 to 1.9.0 the production build stopped working.

Specifically we get this error in the browser console:

Uncaught TypeError: Cannot read properties of undefined (reading 'sign')

Looking into the code that causes this it is minified but here is an excerpt:

module.exports?module.exports:globalThis.nacl=globalThis.nacl||{});const nacl=globalThis.nacl;nacl.sign.keyPair.fromSeed,nacl.sign.detached,nacl.sign.detached.verify,nacl.randomBytes;var NKeysErrorCode;(function(_i){_i.InvalidPrefixByte="nkeys: invalid prefix byte",_i.InvalidKey="nkeys:

It is the nacl.sign that fails. We tracked down why this is used and got to the nkeys package which in turn is used by nats.ws. Pinning the version of nkeys to 1.0.0-9 when using nats.ws 1.9.0 does not work (nats.ws 1.8.1 uses nkeys 1.0.0-9 and it does work).

So for that reason I think the problem is not with nkeys itself but rather how nats.ws does its call into nkeys.

@jonaskello
Copy link
Author

jonaskello commented Sep 26, 2022

Looking a bit deeper I found this commit which in 1.9.0 upgrades nats-base-client. This dependency seems to not use npm so when I pin the version of nkeys in package.json using the resolutions key this probably has no effect since the dependency is via an URL rather than an npm package (it's using deno stuff rather then npm).

As the depencney is via an url, I guess the nats-base-client code gets built in to this package and we cannot override it. Since nats-base-client 1.8.0 uses nkeys 1.0.3 the original source of the error can be from nkeys 1.0.3.

@aricart
Copy link
Member

aricart commented Sep 26, 2022

@jonaskello From your description above, I don't think you are picking the esm version of the library, as you are showing modules etc.
Doing a quick:

mkdir test; cd test; npm init -y; npm install nats.ws and then inspecting node_modules/nats.ws/esm/nats.js I see the full functionality for nkeys built-in. I think your bundler is picking up the cjs. Note that nats.ws/esm/nats.js is fully bundled.

about line 6264

...
    sign(_) {
        if (!this.publicKey) {
            throw new NKeysError(NKeysErrorCode.ClearedPair);
        }
        throw new NKeysError(NKeysErrorCode.CannotSign);
    }
...

@aricart aricart closed this as completed Sep 26, 2022
@aricart aricart reopened this Sep 26, 2022
@jonaskello
Copy link
Author

jonaskello commented Sep 27, 2022

Yes it does seem to have something to do with a global version of nkeys. However I'm not sure why 1.8.1 works and 1.9.0 does not, leaving all other things equal. Did the way the esm module is exposed change between these versions?

Here is a small bash script that reproduces the error. It will create a small project, bundle it with vite build and then serve it from a static web server. When you then start the web browser on localhost:3000 is should give you the error in the browser console.

npm init -y
npm install --save vite nats.ws
npm install --save-dev serve
cat<<EOF>./index.html
<script type="module" src="index.js"></script>
EOF
cat<<EOF>./index.js
import { connect } from "nats.ws";
console.log("Hello");
EOF
npx vite build
cd dist
npx serve
cd ..

@aricart
Copy link
Member

aricart commented Sep 27, 2022

If you look at the sources generated by vite, the full source code is about 30,194 bytes or so.
If I minify the source for nats.js it is 146,690. Their tree shaker is getting rid of a lot. Not sure why, but for the moment, you can get unblocked if you do:

npm init -y
npm install --save vite nats.ws
npm install --save-dev serve
cp node_modules/nats.ws/esm/nats.js ./nats.js
cat<<EOF>./index.html
<script type="module" src="index.js"></script>
EOF
cat<<EOF>./index.js
import { connect } from "./nats.js";
connect().then((nc) => {
	console.log(`connected to ${nc.getServer()}`);
})
EOF
npx vite build
cd dist
npx serve

@sdenovan
Copy link

I'm seeing the same error when importing nats.ws into a MeteorJS codebase. Tracing the source of esm/nats.js (1.10.2) at line 4655:

(function(nacl) {
... // Jump to the end of the function at line 5940)
})(typeof module !== 'undefined' && module.exports ? module.exports : globalThis.nacl = globalThis.nacl || {});
const nacl = globalThis.nacl;

Reviewing the ternary clause (above) that determines what to pass as nacl to the function, if module.exports is defined, nacl will be written into module.exports and globalThis.nacl won't be reached in the ternary else clause to be defined. So line 5941 (above, but repasted here) is assigning nacl to undefined when module.exports is defined:

const nacl = globalThis.nacl;

And the first attempt to work with nacl on line 5943 results in .sign going against the undefined nacl object:

const nacl = globalThis.nacl; // globalThis.nacl isn't defined when module.exports is defined
const denoHelper = {
    fromSeed: nacl.sign.keyPair.fromSeed, // nacl is undefined because it was written into module.exports, not globalThis.nacl
    sign: nacl.sign.detached,
    verify: nacl.sign.detached.verify,
    randomBytes: nacl.randomBytes
};

I believe the fix is for the nacl assignment to mirror the ternary condition. So line 5941 would become:

const nacl = typeof module !== 'undefined' && module.exports ? module.exports : globalThis.nacl;

@sdenovan
Copy link

sdenovan commented Dec 15, 2022

I traced this to nkeys and submitted the PR referenced just above this comment.

@aricart
Copy link
Member

aricart commented Dec 21, 2022

@sdenovan thank you for the fix. @bgdnxt thank you for your patience. I am publishing a new release of the javascript stack.

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

No branches or pull requests

4 participants
@jonaskello @aricart @sdenovan and others