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 original libphonenumber from dist/ #226

Closed
wants to merge 6 commits into from

Conversation

patw0929
Copy link
Owner

@patw0929 patw0929 commented Jul 15, 2018

Related to #222.

Updated:

The usage of react-intl-tel-input which has dynamic importing feature:

  1. yarn add react-intl-tel-input
  2. Demo code will like this (same as before, but without utilsScript):
import React from 'react';
import ReactDOM from 'react-dom';
import IntlTelInput from 'react-intl-tel-input';
import 'react-intl-tel-input/dist/main.css';
import 'react-intl-tel-input/dist/libphonenumber.js';

ReactDOM.render(
  <IntlTelInput
    preferredCountries={['tw']}
    css={['intl-tel-input', 'form-control']}
  />,
  document.getElementById('root')
);
  1. libphonenumber.js will be loaded dynamically.

For dynamic import libphonenumber.js, we should not copy the original libphonenumber.js to dist/ folder.

The usage of react-intl-tel-input which has dynamic importing feature:

  1. yarn add react-intl-tel-input
  2. Use babel-plugin-syntax-dynamic-import in webpack dev mode.
  3. Use CopyWebpackPlugin to copy libphonenumber.js to local (e.g., new CopyWebpackPlugin([{ from: 'node_modules/react-intl-tel-input/dist/libphonenumber.js', to: './' }]), in dev & production webpack plugin settings.
  4. Demo code should like this (without importing libphonenumber manually):
import React from 'react';
import ReactDOM from 'react-dom';
import IntlTelInput from 'react-intl-tel-input';
import 'react-intl-tel-input/dist/main.css';

ReactDOM.render(
  <IntlTelInput
    preferredCountries={['tw']}
    css={['intl-tel-input', 'form-control']}
  />,
  document.getElementById('root')
);
  1. libphonenumber.js will be loaded dynamically.

image


Problem

  1. The dynamic import path is only at root (.), need to find out the way to custom by developer (e.g., static/js).
  2. The usage of react-intl-tel-input seems too complicated now, is it really the right way?

@coveralls
Copy link

coveralls commented Jul 15, 2018

Coverage Status

Coverage remained the same at 93.344% when pulling 79cb2f2 on feature/remove-libphonenumber-from-dist into e368560 on master.

@superhit0
Copy link
Collaborator

LGTM

@patw0929
Copy link
Owner Author

@superhit0 have any idea about the usage problem? It seems complicated now.
And the import path is also a problem, it cannot change by setting, the path is always ".".

@superhit0
Copy link
Collaborator

The reason behind removal of 'libphonenumber.js'

  • If the user is using the libphonenumber in the repo, there is no need for him to manually link it in his/her code, we should bundle it our selves in our code.
  • Still the utilScript option is there, for him to manually link his/her custom made utilscript.
  • The path problem to me looks ok, because even in normal code we use import '../some-folder/some-js-file' and it is not configurable & kind of hard coded & to me its ok because the dynamic import is a fallback scenario where if the developer by mistake does not provide utilscript, we fall back to our own libphonenumber, which we are anyways shipping with our code.

@patw0929 I am open to discussion on this & I think we should proceed when we are clear on this.

@superhit0
Copy link
Collaborator

@patw0929 any comments?

@patw0929
Copy link
Owner Author

@superhit0 Thank you for your feedback! Sorry, I just got off work. 😅
Let me clarify my concern:

  1. If the user is using the libphonenumber in the repo, there is no need for him to manually link it in his/her code, we should bundle it our selves in our code.

What is the manually link mean? Is it like import 'react-intl-tel-input/dist/libphonenumber.js?

  1. If user want to use utilsScript way to load libphonenumber like before, he/she can't use the libphonenumber.js in dist folder. Because the libphonenumber.js is not the original one, it is bundled by webpack.

2018-07-15 11 44 11

  1. About the path issue, for some reason, someone might like to make all the .js file in js/ folder, but it seems that it can only put libphonenumber.js at root now.

For me, the main concern is the simple usage for developers like before.
Thanks. 😄

@superhit0
Copy link
Collaborator

superhit0 commented Jul 16, 2018

  1. What is the manually link mean? Is it like import 'react-intl-tel-input/dist/libphonenumber.js?

Yes.

  1. If user want to use utilsScript way to load libphonenumber like before, he/she can't use the libphonenumber.js in dist folder. Because the libphonenumber.js is not the original one, it is bundled by webpack.

The user should not be able to do that, because if the user wants to use the libphonenumber from our repo only, he can simply use the one which is bundled with our webpack. Why the user should put extra effort just to use the code that is supplied by us. In this way the main project code of his will have less boilerplate code, like the import statement or the extra tag of utilscript. But still if the user wants his own set of validations like his custom libphonenumber, he may still use his custom libphonenumber the old way. And, if you notice that if you use 2 IntelInputApp in a page, the libphonenumber is downloaded twice, we can limit that to 1 by using lazy-once in webpack.

  1. About the path issue, for some reason, someone might like to make all the .js file in js/ folder, but it seems that it can only put libphonenumber.js at root now.

The user can simply use webpack output option for this in his/her repo.

This was my reason for removing libphonenumber. Lets discuss this further if it causes some issues.

😄 🤔

@superhit0
Copy link
Collaborator

Also, I don't know why email notifications are not working for me. Are you on Linkedin or some other network? Where we can ping each other.
I am generally online most of the time. 😄

@patw0929 patw0929 force-pushed the feature/remove-libphonenumber-from-dist branch from dbe37d9 to 79cb2f2 Compare July 16, 2018 15:19
@patw0929
Copy link
Owner Author

OK, I just added lazy-once in new commit.
If possible, could you make some demo to clarify how to use react-intl-tel-input (based on this PR) in new way? The demo code might be mentioned at README too. I want to make sure there is no other usage issues we missed.

And here is my Linkedin: https://www.linkedin.com/in/patrickpatw/
But I am not often on Linkedin at my work time.
Maybe use Gitter for this project?

@superhit0
Copy link
Collaborator

superhit0 commented Jul 16, 2018

ok I will do that tomorrow.

Sure anything works for me as long as its instant messaging.

@superhit0
Copy link
Collaborator

superhit0 commented Aug 9, 2018

@patw0929 I was going through the change and found that the libphonenumber in dist that we get from webpack is the same as we get when we simply copy paste. [Please confirm this from your end as well]

I think we can go ahead with this change without affecting much users as still they will have the option to import libphonenumber from dist folder and even if they dont, webpack will automatically get it through their bundling system.

@superhit0
Copy link
Collaborator

@patw0929 any update on this?

@nutboltu
Copy link
Collaborator

nutboltu commented Dec 1, 2018

@patw0929 , @superhit0 Can we merge this PR?

@superhit0
Copy link
Collaborator

@patw0929 our release has been delayed a lot. Can we please wrap this up?

@patw0929
Copy link
Owner Author

Close it because #242 was merged.

@patw0929 patw0929 closed this Dec 16, 2018
@patw0929 patw0929 deleted the feature/remove-libphonenumber-from-dist branch January 5, 2019 02:36
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.

None yet

4 participants