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

Defer importing ky-universal until actually needed #35

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

Peeja
Copy link
Contributor

@Peeja Peeja commented Mar 31, 2023

This accomplishes two things:

  • It prevents a dangling promise with an import in it. If the module is loaded (queuing up the import promise) and then no clients are ever used, and thus never awaited, the import can happen too late. In particular, Jest will break if the import happens after the test is complete.

  • It avoids a dynamic import altogether when the client isn't needed. In particular, Jest uses Node's VM API which only experimentally supports dynamic imports (and ESM modules altogether). This change means that merely loading the http-client module doesn't require enabling that experimental support.

Fixes #34


@dlongley This should be a non-breaking change. I've used it locally against my repro case and it seems to work great. Let me know if there's anything you'd like me to change (or have at it yourself if that's easier).

There's only one concern I have about it, which is that the error, when it does happen, is now harder to spot. If I leave --experimental-vm-modules off and try to expand something with a URL context, as in:

import { expand } from "jsonld";

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

…now it fails much more cryptically:

jsonld.InvalidUrl: Dereferencing a URL did not result in a valid JSON-LD object. Possible causes are an inaccessible URL perhaps due to a same-origin policy (ensure the server uses CORS if you are using client-side JavaScript), too many redirects, a non-JSON response, or more than one HTTP Link Header was provided for a remote context.

That's because the error now happens during httpClient.get(url, options), and jsonld interprets that as an HTTP problem. There's a chain of causes on the error that will lead back to a meaningful error from Jest again, but it doesn't print when you run it, because it's not in the error's message. I'm not sure what the best move to deal with that is. In any case, I'm curious what you think!

This accomplishes two things:

* It prevents a dangling promise with an import in it. If the module is
  loaded (queuing up the import promise) and then no clients are ever
  used, and thus never awaited, the import can happen too late. In
  particular, Jest will break if the import happens after the test is
  complete.

* It avoids a dynamic import altogether when the client isn't needed. In
  particular, Jest uses Node's VM API which only experimentally supports
  dynamic imports (and ESM modules altogether). This change means that
  merely loading the http-client module doesn't require enabling that
  experimental support.
@dlongley dlongley requested review from dlongley and davidlehn April 1, 2023 14:51
Copy link
Member

@dlongley dlongley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@davidlehn, this looks like it works to me and doesn't have a breaking change. It will need a couple of style fixes once we pull it in and a CHANGELOG entry for a minor revision, but otherwise LGTM. Thoughts?

@dlongley
Copy link
Member

dlongley commented Apr 4, 2023

A possible solution the Jest error reporting problem (if we choose to do something about it at all right now) would be to check the error chain and surface the cause of the error more directly. I don't think we need to do that right now due to the obscurity of the error and the fact that the information is, in fact, present in the error chain if inspected.

@sroertgen
Copy link

Thanks for this @Peeja!
Ran into the same problem with Jest today and I'm so glad you already worked on a fix!

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 this pull request may close these issues.

Consider lazy loading ky-universal
4 participants