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

Remove the dependency on window #348

Open
mcataford opened this issue Jun 15, 2020 · 4 comments
Open

Remove the dependency on window #348

mcataford opened this issue Jun 15, 2020 · 4 comments

Comments

@mcataford
Copy link
Collaborator

Phone number validation happens via utilities that are asynchronously loaded into window by libphonenumber-js-utils. This has been shown to cause problems in certain test runners (#347) and is generally a practice that should be avoided because it puts critical business logic in a place that the library has very little control over (the window global).

This issue exists to track refactoring work around not having that asynchronous dependency.

Expected Behavior

The library should not rely on window for critical functionality and should not asynchronously import packages.

Current Behavior

libphonenumber-js-utils is imported asynchronously for its side-effects, and critical logic is held in window.

Possible Solution

Refactor both react-intl-tel-input and libphonenumber-js-utils so that there is a synchronous path to import and use those utilities without relying on window as a communications channel.

Environment

  • Version:
    (Presumably) All.
  • Browser:
    (Presumably) All.
@mcataford
Copy link
Collaborator Author

mcataford commented Jun 17, 2020

@patw0929 I was wondering if you had any thoughts on this.

From what I saw, the original libphonenumber implementation of the functions react-intl-tel-input uses hasn't really moved in two years, so I'd be tempted to sever the tie to the Closure Compiler, trim libphonenumber-js-utils down to a library that can be synchronously imported like any other and that doesn't put critical logic in window. 🤔

We could still pull the same data, but have pure JS business logic and a build process that bundles it nicely. It would certain make the whole thing more accessible (libphonenumber-js-utils included) and would be a good step forward in optimizing react-intl-tel-input from a code complexity and general reliance on external resources point-of-view. I'd be more than happy to give that libphonenumber-js-utils work a shot, too.

@BramKaashoek
Copy link

I am experiencing segfaults in my CI/CD, probably because of this dynamic import as shown in nodejs/node#27492.

@patw0929
Copy link
Owner

@mcataford
Nice idea!

I tried to make it easier to use & maintain (and keeping the functionality) before, but no luck (and no time).
I am wondering if there is any other js libphonenumber project already make that & can work with react-intl-tel-input.

Again, thank you for the continued contributions! 🙇
(Invited you as libphonenumber-js-utils's collaborator too.)

@mcataford
Copy link
Collaborator Author

I quickly surveyed the landscape of libphonenumber ports, a lot of them seem to go the Closure Compiler route. Since libphonenumber-js-utils has some dependents, I think it's worthwhile to rework it either way, but I'll see if there's any likely candidates for react-intl-tel-input depending on work complexity! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants