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

Allow promise-like objects as loadUtilsOnInit option #1878

Closed

Conversation

MarcoGlauser
Copy link
Contributor

Related to #1877
This solves the issue that angular can't load the utils script because zone.js wraps promises.

The utils loader currently does an instanceof check against the builtin Promise type. This doesn't work with wrapped promises, bluebird promises or other custom promises.

By wrapping the source() result in a Promise.reslove(), instead of the instanceof check, we're making sure that we're working with a promise.

I've added a specific testcase for a custom thenable that works with this patch.

@jackocnr
Copy link
Owner

Checking with @Mr0grog, who implemented this code. Any thoughts before I merge?

loadCall = source();
if (!(loadCall instanceof Promise)) {
throw new TypeError(`The function passed to loadUtils must return a promise for the utilities module, not ${typeof loadCall}`);
}
loadCall = Promise.resolve(source());
Copy link
Contributor

Choose a reason for hiding this comment

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

@jackocnr This seems fine to me. 👍 The main thing worth considering is that this will also allow the loadUtilsOnInit function to return the utils module object directly instead of just as a promise. That is, you can now do:

intlTelInput(inputElement, {
  // No promises involved!
  loadUtilsOnInit: () => ({
    default: { /* Locally defined utils module, or a mock, or whatever here */ }
  })
});

But that flexibility might be useful! So I think this is probably a good change. (Side note: the function can technically return anything with this change, but we have code later [line 2168] to validate that it at least vaguely looks like a JS module object, so that should be OK.)

OTOH, if you want to keep requiring that this returns a thennable (the more generic form of a JS promise), then the right canonical check would be something like:

loadCall = source();
if (typeof loadCall?.then !== 'function') {
  // Throw an error (or maybe return a rejection instead)
}

…but like I said, I think the added flexibility in this change as-is is probably better.

@jackocnr
Copy link
Owner

jackocnr commented Dec 1, 2024

Manually resolved conflicts and merged. Thank you so much. Released in v24.8.1.

@jackocnr jackocnr closed this Dec 1, 2024
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.

3 participants