-
Notifications
You must be signed in to change notification settings - Fork 11
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
fix undefined suiteContext2020 object when importing with jest #18
fix undefined suiteContext2020 object when importing with jest #18
Conversation
d1833f2
to
7b2b1de
Compare
7b2b1de
to
32a1681
Compare
This is probably some module/cjs/esm/esm.js mixing issue with the tooling. This single line difference is probably to blame although it looks like it should function the same: https://github.com/digitalbazaar/ed25519-signature-2018-context/blob/master/js/index.js#L12 https://github.com/digitalbazaar/ed25519-signature-2020-context/blob/master/js/index.js#L12 May be better to fix the issue over in the 2020 context package if the 2018 one works. |
And if anyone wants to properly debug this to understand the failure and what a proper fix is... well that would be nice too. ;-) |
Correct, my hunch is that this is related to the way things are exported, and jest does not pick it up correctly. I am actually facing other issues with other packages where the exported classes are empty and the methods undefined. I spent a good deal of time getting
I don't really understand how you expose things ESM or CJS so I don't feel confident spending too much time on it, plus it's a pattern across all the libraries so it would be a big change that I cannot impose :)
I think I tried that but it didn't change anything. I will try again and confirm. For instance I had to change
As otherwise I was also getting an error:
Again these are all jest concerns (at least I think so, I haven't tested the final browser build yet, but it was working fine as a nodejs script). |
Ok I tried again and that does not work |
So to get to a working verification of Ed25519 signed credential with Jest, I need to:
which means this PR is actually irrelevant now
Where I am preventing some sort of hoisting issue by wrapping the
I will try and see how it behaves without the changes in the browser to isolate if this is only an issue with Jest. |
We should also note that #17 should fix this (but will involve a major version bump). |
ok I'm still trying to test the browser build I have a issue with a dependency of jsonld-signature which does not work with a rollup plugin so I haven't tested. |
After quite some hacking I have finally got a browser build and it verifies my document as expected. I'll try and see if there is a high level configuration for jest that could work. |
@dlongley so with the I had to take each (all digital bazaar libraries)
To note, this was also required to be able to build the project afterwards with rollup. Apparently jest is supposed to support Rollup is also supposed to pick it up: rollup/plugins#208 but I've had the following if not done:
|
Sorry for all the hassle with this code! In theory it should "just work" but apparently that is not at all the case at the moment. Looking at jest release notes, one would think it should handle what is going on here. But I see there are some jest/node issues related to imports. Maybe the double import thing you initially mentioned is part of that? Maybe serial vs parallel testing would help there? Good to know the module conversion branch helps. I had thought all the modern module handling tools would work with just "exports" without "main". I had hoped to not have to have both. The context packages are all a bit mixed up at the moment. They all do a similar thing of transforming cjs to esm. But they were written a while back and only have a "module" field. So bundlers will use the esm code, but node always uses cjs version. It did appear they all were working anyway so I was pushing off fixing all context packages until later. They will probably all switch around to module code that generates cjs. Or maybe just drop cjs? Not sure. So in theory we're working towards all module code for the stack. And hopefully tools catch up with the style we're using. I'm not sure what to do if some tools are not quite there. Suggestions welcome. |
Tomorrow I'll see how I can update Jest and Rollup, but I'm expecting trouble on that front which is why I had settled with the older versions. |
It looks like the support has just landed for Typescript: microsoft/TypeScript#33079 (comment). The announcement has been made on the 11th of May: https://devblogs.microsoft.com/typescript/announcing-typescript-4-7-rc/#esm-nodejs Initially I have faced this error:
Not only with this package but also with So modifying the And while this resolved importing the first level dependency, it seems it does not resolve correctly its own dependencies (it chokes on LIVE NOTE: I have applied the fix to the child deps, and now I am facing the issue with Ed25519VerificationKey2020 again, so it's pretty inconsistent and not really reliable/predictable. I swear I've managed to get 1 verification go through... And I haven't started looking at rollup yet. All in all it does not look like there is a straightforward support yet. I could prepare a repro if you wanted to work with it, but I also have deadlines coming up. In the typescript documentation: https://devblogs.microsoft.com/typescript/announcing-typescript-4-7-beta/#package-json-exports-imports-and-self-referencing they still seem to use |
FWIW, I have forked all the involved packages in my project and added the I'm going to close this PR, but happy to keep the conversation rolling. |
When consuming with jest test runner, I was getting the following error:
coming from this line:
There was strangely no issue with the 2018 one. I noticed that
ed25519-signature-2020-context
was being called twice, the first time the value was undefined, the second it was the correct value.I am not sure what exactly was happening, but this seems to fix it without changing the API of the package.