Support async functions for loading the utils script

[Mr0grog]

Fixes #1816.

Sorry this was a bit of a long time coming! I ran into a few messy issues with Jest (partially detailed in #1630) that caused me to have to do the side-note item (making sure there are no unhandled promises) to get the existing tests to run in Jest, but then I ran into complex dynamic import problems in Jest and went back to Jasmine for the new tests, and then eventually figured out the right workarounds for transforming the imports in Jest. 😩 Hopefully these commits are clearly factored enough to better evaluate the different pieces of work.

Anyway, this PR does a few main things:

1. Make sure the library never leaves promises in an unhandle-able state (thereby sometimes causing internal exceptions that can never be addressed by developers using the package). Specifically, the loadUtils() static function returns a promise, but when a new instance calls that function to lazy load the utils, promise rejections were never handled (the instances do get informed about the failure, but through a complex setup that leaves the failure unhandled).

   I’ve changed things to:

   • Always handle that rejection when calling loadUtils() internally. When users call it directly, however, it is left unhandled and they need to make sure to cover it.
   • Pass through the actual error that called the rejection. Previously, you’d just get undefined as the reason, and now you get a proper error object with more info, like "couldn’t import ” or “syntax error on line ”.

   This felt like the minimum change to solve the unhandle-able errors issue, although I think an ideal solution might be a bit different (see footnotes).

   I also tweaked the types for startedLoadingAutoCountry and startedLoadingUtilsScript to always be booleans (previously they were <not-present> | true) because I noticed some tests were broken because the test cleanup resets them to false instead of resetting them by deleting them (only visible as a fail if those tests run first). This should be pretty minor, I hope.

2. Added a server for the Jasmine tests or for viewing the demo.html file over HTTP, so you can test dynamic imports of the utils. (Trying to load from a file:// URL is not allowed in most browsers for security reasons.)

   The server starts automatically for Jasmine, or you can run npm run server or npx grunt jasmine:interactive to start a long-running server on port 8000. Then you can load up http://localhost:8000/demo.html or http://localhost:8000/spec.html to debug in a browser. See changes to CONTRIBUTING.md for more description.

3. Added support for loadUtils() to take an async function that resolves to the loaded module and for the utilsScript option to do the same. Also renamed the option from utilsScript → loadUtilsOnInit, per the discussion.

   This includes expanded tests to cover actually loading (or failing to load) the code.

4. Ported the loadUtilsOnInit option and loadUtils static function tests from Jasmine → Jest.

   As part of this, I moved the Jest config into a separate file so I could add comments, but I can put it back in package.json if you’d prefer.

---

Footnote

How load failures are handled: a page with 2 inputs on it that never explicitly handles rejections of <instance>.promise will log 2 errors (unhandled rejections) to the browser console. That is, this setup:

const instance1 = intlTelInput(inputElement1, { loadUtilsOnInit: "path/to/utils.js" });
const instance2 = intlTelInput(inputElement2, { loadUtilsOnInit: "path/to/utils.js" });
// The problem is that we don't have this code:
// instance1.promise.catch(() => doSomething());
// instance2.promise.catch(() => doSomething());

Will log two errors when "path/to/utils.js" fails to load. That’s better than it used to be, when three were logged and one of those could never be suppressed (the one coming from loadUtils() directly instead of from <instance>.promise).

However, you get the same error N times, once for each instance on the page, seems annoying. Handling all errors correctly also means developers have to attached handlers to <instance>.promise, which seems like a pain. I think a nicer solution here would be:

• Add a intlTelInput.handleError(handlerFunction) static method, or maybe an "error" event (or a differently named event that doesn’t conflict with the browser’s built-in one) that allows people to install a global error handler. If users don’t install their own handler, we’d have a default one that logs the error. But this gives people a nice, singular hook to send errors to monitoring services like Sentry/Datadog/etc. This could also handle errors from autoloading the country, etc, which has similar issues.

• Ensure <instance>.promise is always handled with a no-op handler. Developers can still catch rejections from it, but if they don’t, they won’t get unhandled rejection errors in their console.

This way an error loading the utils script only gets logged once, instead of for every phone input on the page, and there is a clear place to hook up and handle global errors.

[jackocnr]

This is really incredible work. This must be the most professional and comprehensive PR I've ever received! Thank you so much for your hard work here - it is really appreciated. This is a great step forward for the project. 💐

BTW, I agree with your footnote suggestions and would welcome a PR to that effect if you have time.

[jackocnr]

Released in v24.6.0

[Mr0grog]

I agree with your footnote suggestions and would welcome a PR to that effect if you have time.

Will see if I can find some time for it later this week. 👍

[jackocnr]

@Mr0grog do you know what, on second thought, I don't think it's worth adding a new static method just for the edge case of people who (A) have multiple instances, and (B) use a bad utils URL, just to prevent them from receiving a duplicate error message. The duplicate error message is really not a problem for me in this case.

I think the only thing that's important is that it's possible for people to catch this error if they want to. I believe this is already possible, but forgive me, I don't have time to double-check right now - can you confirm? (we should update the readme to make this clear)

Additionally, in the next major release, I plan to simplify this utils loading code so that both loadUtilsOnInit and loadUtils stop accepting a string argument and only accept the function that returns a promise which resolves to the utils module. That way, we can remove the plugin's dynamic import which will make things more straight forward.

[Mr0grog]

Fair enough! FWIW, I do think the idea was about more than those two things, since it also covers errors from the geo lookup, too (the way .promise is set up, you will only ever receive an error from either utils loading or geo lookup, but not both — though I appreciate that this is also probably an unusual case).

I believe [catching the error if you want] is already possible… can you confirm? (we should update the readme to make this clear)

That was what the first commit in this PR does. You have to call instance.promise.catch(), but that will now trap the error (caveat the issue above if you have multiple errors, those will just be hidden from you and not surfaced in any way right now, I think).

simplify the utils loading code so that both loadUtilsOnInit and loadUtils stop accepting a string argument… we can remove the plugin's dynamic import which will make things more straight forward.

FWIW, you'd still need all the same error handling and so on, and this would not have resolved most of the issues I ran into with Jest.

[jackocnr]

You have to call instance.promise.catch()

Right, perfect, thanks.

caveat the issue above if you have multiple errors, those will just be hidden from you

Ok, good to know, but I'm not worried about handling this just yet.

FWIW, you'd still need all the same error handling and so on

👍🏻

and this would not have resolved most of the issues I ran into with Jest.

Sorry, I'm confused - are you saying that this proposed change would make the situation worse?

[Mr0grog]

and this would not have resolved most of the issues I ran into with Jest.

Sorry, I'm confused - are you saying that this proposed change would make the situation worse?

No, sorry for being unclear!

It would definitely eliminate one conditional branch from loadUtils, and that does make for simpler code and fewer tests. 👍 But most of the complexity comes after turning the string into a call to import(), which is all stuff you’d have to keep. What I meant is that it struck me as relatively small gain for a breaking change.

As for Jest in particular, this would eliminate the need for the --experimental-vm-modules option in .npmrc, but it would still need the special transforms for utils.js, which were really the thing that took the most work for me to figure out and get set up right. (Assuming you’d want at least one test that actually loads utils.js instead of a mock, so you test that the real thing does in fact work.)

[Mr0grog]

That’s not to say there aren’t things you could do to simplify this functionality. There’s some stuff I think I could have done more nicely in loadUtils in this PR, and I think you could probably simplify a lot of other bits (and fix some weird edge cases) by making loadUtils() always return a promise. You could maybe also just drop the removeImport stuff from grunt-replace with relatively low impact.