digitalbazaar/http-client

[malins]

Hello,

I'm having problems to test my code using Jest because jsonld library is using digitalbazaar/http-client, which in turn uses ky-universal/ky, which is only available as ESM build and does a dynamic import.

The underlying V8/node bug is described here: nodejs/node#35889

What's the reason to use digitalbazaar/http-client and not plain fetch (with node-fetch on nodejs)? digitalbazaar/http-client seems to be quite a exotic dependency of your package.

Thank you!

[davidlehn]

As I suggested elsewhere, you might try the solution they recommend: https://jestjs.io/docs/ecmascript-modules.

We use our own http-client library in many of our other projects and it fits our use case and historically handled some compatibility issues like using node-fetch. A fetch based documentLoader is certainly possible and you can write and use one with this library. We haven't spent the effort on making one ourselves. There might be an issue of it always loading the default, and hitting the issue you see, so maybe some overrides would be needed to stop that.

I think some documentLoader code refactoring is needed in general, to address use cases like this and other issues, but it hasn't been a priority.

[malins]

Thank you for your reply. I didn't notice that this http-client is your own package :-) I already tried that experimental flag, but that doesn't solve the problem, as ky-universal does a dynamic import (await import(...)) which leads to a segfault when running code inside the nodejs vm module (Jest uses vm module for isolating test runs from each other).

As the node document loader is imported by default, the only solution i've found so far is to patch your package after installation.

Once again, thank you for your reply.

[Peeja]

I'm having the same issue, and m-ld in general has run into this. m-ld currently has its own fork of jsonld.js to solve this, but it's obviously not super well maintained. The only thing it does is leave out the http-client.

The best solution would probably be for documentLoader to support fetch, but barring that, an intermediate solution would be for http-client to be optional and only used if you attempt to use the default documentLoader.

[dlongley]

@Peeja,

I agree that it would be a good idea to reduce the number of libraries required here (or do some kind of lazy loading) -- provided that it doesn't significantly increase the maintenance burden. That's usually a good practice in general.

However, I believe that this extra effort also shouldn't be a pressing matter here or elsewhere in the JavaScript ecosystem; elevating its priority diverts efforts that are being made in other important areas. I certainly sympathize with struggling developers -- but the cause of this perceived need for prioritization should be examined to see if there's a more efficient solution.

In my view, the best solution is for any modern tooling that purports to work with JavaScript but does not support features of the ECMAScript standard (such as ESM) should be updated such that they do. It's certainly less efficient for developers to go around either forking and maintaining or requesting modifications to every ESM library they use because it won't run with their tooling -- especially when those libraries follow the ECMAScript standard.

It seems to me that whatever tooling (usually TypeScript and / or Jest) is suffering from a lack of ESM support should be updated post-haste -- and the rest of the decentralized ecosystem that is otherwise compliant could be left alone, as it would "just work".

[Peeja]

@dlongley It's not TypeScript or Jest, though, it's Node. ESM support in Node is still experimental. I agree that solving the underlying issue is more effective, but I'm not sure it's reasonable to expect Node itself to move any faster than it already is in that direction.

[dlongley]

@Peeja,

It's not TypeScript or Jest, though, it's Node.

And Node community would say it's v8 (and they wouldn't be wrong). My view is that each of those places is a potential bottleneck for addressing the problem. Clearly the most ideal fix is in v8 -- it solves the problem for everyone (once brought into Node, etc.). The next best place for a fix would be Node, and then Jest after that. The worst place (bottleneck-wise) to do the fix would be every ESM library that any developer using Jest depends on (directly or indirectly).

I'm not sure it's reasonable to expect Node itself to move any faster than it already is in that direction.

Sure, I'm with you on that. But, given that this problem has now been an issue for at least 2 years, I think the Jest community should move to provide some fix / temporary alternative mode to address the issue. Other testing frameworks (and many applications) are able to work with ESM without issue, including running code in Node. I think that's better than struggling developers going from library to library asking each maintainer to make changes to accommodate Jest (+/- Node +/- v8). Hopefully my position makes sense. Clearly none of this is ideal.

[Peeja]

@dlongley I hear what you're saying. I'm not clear what you think the pragmatic, short term solution is, though? Just…don't just jsonld.js at all for a year or two? Keep maintaining our own fork? I'm not sure how else to get around it.

As an entirely separate matter, though, it's frustrating that there's no way to use jsonld.js without bringing along an HTTP client.

[Peeja]

Oh, hang on, I think I see the problem! This import() isn't in an ESM module at all: it's in a CommonJS file. I think that's throwing off downstream tools which don't expect to do ESM-y things with that module, even if they support them. Is there maybe something wrong with the build chain that's producing that?

Ah, false alarm, that is the correct way to get to ESM code from CommonJS, I had it in my head it had to be a form of require() instead.

[Peeja]

Aha! I think I've found the actual issue now.

First, you need to run with NODE_OPTIONS=--experimental-vm-modules, as Jest says to (and as mentioned above). But that still was giving me:

ReferenceError: You are trying to `import` a file after the Jest environment has been torn down.
# or sometimes
Error: Test environment has been torn down 

Here's what's happening. jsonld.js requires http-client, which then dynamic-imports ky-universal, which then dynamic-imports ky. Being CommonJS, http-client can't actually wait for ky-universal to load before the module completes its top-level execution. So now we have a queued Promise for ky-universal, and while that's queued, jsonld finishes requiring http-client and the application finishes requiring jsonld. If that application is a Jest test, and if that test doesn't do much before completing, it can easily complete and let the test tear down before that queued import actually kicks off. And that may actually be fine, except that's when we hit ky-universal's import("ky"), and that causes an error. We never awaited the ky-universal Promise during the test.

Notably, this test will reproduce the issue:

import { expand } from "jsonld";

test("repro", async () => {
  await expand({
    abc: "123",
  });
});

…while this one won't:

import { expand } from "jsonld";

test("repro", async () => {
  await expand({
    "@context": "http://schema.org/",
    abc: "123",
  });
});

That's because using a URL context involves the ky-universal dependency, so we end up awaiting the ky-universal Promise as part of expand(). We can also resolve the problem by doing:

import { expand } from "jsonld";
import { kyPromise } from "@digitalbazaar/http-client";

beforeAll(async () => {
  await kyPromise;
});

test("repro", async () => {
  await expand({
    abc: "123",
  });
});

…which is pretty awkward, but does demonstrate that this is the issue.

Off the top of my head, I think the best solution here is to make the import('ky-universal') lazier so the Promise always ends up as part of some other promise that the API returns, rather than it happening at the top level of the module. But I see there's a good deal of infrastructure there with the way those promises work, so I may be missing a reason why that would be a poor choice.

In any case, if you have an opinion on the strategy here, I'm happy to make a PR to address it.

[dlongley]

@Peeja,

Thanks, great analysis!

It seems that a PR to http-client could be made that did something to the internals ... like making _createHttpClient take a function that returned a (possibly memoized) kyPromise rather than taking a promise directly. That would allow the original import of ky-universal to also be moved within that function for the default case where the parent is the original / base ky instance:

https://github.com/digitalbazaar/http-client/blob/v3.3.0/lib/httpClient.js#L35-L51

Unfortunately, it seems this would mean that the kyPromise exported by the library would also need to become such a function, which would be a breaking change. Maybe we can figure out a way around that -- but even if we can't, a major version could be released that avoids the problem you identified.

Any further discussion that relates to http-client specifically should be taken over here: digitalbazaar/http-client#34

[davidlehn]

The deferred dynamic import fix is in @digitalbazaar/[email protected]. It would be helpful for people to test and see if it helps address Jest related issues.

[dmitrizagidulin]

The deferred dynamic import fix is in @digitalbazaar/[email protected]. It would be helpful for people to test and see if it helps address Jest related issues.

I'm testing > @digitalbazaar/[email protected] (4.x actually) in Jest, and it's causing a segmentation fault due to the dynamic import :( (Still giving me a A dynamic import callback was invoked without --experimental-vm-modules error, even though I run jest via NODE_OPTIONS=--experimental-vm-modules.