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

2fa: Update to use upstream phone package again #27902

Merged
merged 7 commits into from
Nov 20, 2019
Merged

Conversation

kraftbj
Copy link
Contributor

@kraftbj kraftbj commented Oct 17, 2018

Changes proposed in this Pull Request

  • Updates Calypso to use upstream phone npm package again.

Originally, we forked the npm package because of slow updates, but the upstream package appears to be updating on a regular basis now.

Testing instructions

Fixes #25163 and possibly others detailed in p4TIVU-98g-p2 but would need testing.

I'm thinking this may be a short-term stop-gap while reviewing if there is a better package to use where we wouldn't have to maintain it.

@kraftbj kraftbj added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Security labels Oct 17, 2018
@matticbot
Copy link
Contributor

@ghost
Copy link

ghost commented Oct 17, 2018

That's a great PR description, thank you so much for your effort!

Generated by 🚫 dangerJS

@blowery
Copy link
Contributor

blowery commented Oct 18, 2018

Nice! Always glad to get back on the mainline for these things.

Looks like there are a couple tests that may need to be updated from this? Or maybe the return changed a bit?

@blowery blowery self-requested a review October 18, 2018 17:35
@kraftbj
Copy link
Contributor Author

kraftbj commented Oct 19, 2018

I'm looking into it. Those numbers fail, but looking at the upstream, they should pass. Trying to verify how we make that check to verify we're not doing anything unexpected.

@kraftbj kraftbj added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 19, 2018
@akirk
Copy link
Member

akirk commented Nov 6, 2018

This should also fix Tunesian phone numbers, see pxLjZ-55p-p2

@liviopv
Copy link

liviopv commented Dec 27, 2018

@kraftbj is there any estimate of when this is going to be merged?

@yoavf
Copy link
Contributor

yoavf commented Dec 27, 2018

@liviopv see #29740 - I think that's a better solution. I can merge after a review is done. Are you using the module elsewhere?

@akirk
Copy link
Member

akirk commented Jan 2, 2019

@blowery
Copy link
Contributor

blowery commented Feb 19, 2019

Wow, those PRs upstream still have not landed.

@akirk
Copy link
Member

akirk commented Oct 15, 2019

Upstream has now moved again but we have a report about a still not fixed Brazilian area code in #35339

I think resyncing our fork, merging that PR from upstream early, and updating our dependency here to continue to use the fork is a viable next step?

@akirk akirk force-pushed the update/phone-validation branch from 9f87532 to a4d6af7 Compare October 17, 2019 10:44
@kraftbj kraftbj requested a review from a team as a code owner October 17, 2019 10:44
@akirk
Copy link
Member

akirk commented Oct 17, 2019

The upstream package is now up to date and has also incorporated the latest Brazilian phone number changes (AfterShip/phone#154), so we can use upstream again.

@akirk akirk added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Blocked / Hold labels Oct 17, 2019
@akirk akirk assigned akirk and unassigned yoavf Oct 17, 2019
@matticbot
Copy link
Contributor

matticbot commented Oct 17, 2019

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~7668 bytes added 📈 [gzipped])

name                   parsed_size           gzip_size
site-blocks                +1789 B  (+0.7%)     +639 B  (+1.0%)
security                   +1789 B  (+0.4%)     +639 B  (+0.6%)
purchases                  +1789 B  (+0.2%)     +639 B  (+0.3%)
privacy                    +1789 B  (+0.8%)     +639 B  (+1.1%)
notification-settings      +1789 B  (+0.6%)     +639 B  (+0.8%)
me                         +1789 B  (+0.8%)     +639 B  (+1.1%)
help                       +1789 B  (+0.4%)     +639 B  (+0.6%)
happychat                  +1789 B  (+0.7%)     +639 B  (+1.0%)
concierge                  +1789 B  (+0.6%)     +639 B  (+0.9%)
checklist                  +1789 B  (+0.6%)     +639 B  (+0.8%)
account-close              +1789 B  (+0.6%)     +639 B  (+0.8%)
account                    +1789 B  (+0.6%)     +639 B  (+0.8%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~1917 bytes added 📈 [gzipped])

name                          parsed_size           gzip_size
async-load-design-playground      +1789 B  (+0.1%)     +639 B  (+0.2%)
async-load-design-blocks          +1789 B  (+0.1%)     +639 B  (+0.1%)
async-load-design                 +1789 B  (+0.1%)     +639 B  (+0.2%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

package.json Outdated
@@ -155,7 +155,7 @@
"page": "1.11.4",
"path-browserify": "1.0.0",
"percentage-regex": "3.0.0",
"phone": "git+https://github.com/Automattic/node-phone.git#v2.4.0-pre-1",
"phone": "2.3.22",
Copy link
Member

Choose a reason for hiding this comment

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

There's now v2.4.0 BTW.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, tests still pass fine, so I've bumped the version.

@akirk akirk force-pushed the update/phone-validation branch from 5251e96 to 934ebe5 Compare October 21, 2019 10:49
Copy link
Contributor

@blowery blowery left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for getting us back on mainline.

@blowery blowery force-pushed the update/phone-validation branch from 934ebe5 to d46d8f4 Compare November 20, 2019 01:06
@blowery
Copy link
Contributor

blowery commented Nov 20, 2019

@akirk I rebased this to pick up the lockfile change and bumped the version to 2.4.2. Could you give it a quick sanity check?

@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 20, 2019
@blowery blowery force-pushed the update/phone-validation branch from 02366c6 to 8ec836c Compare November 20, 2019 12:32
@blowery blowery merged commit 54ce88c into master Nov 20, 2019
@blowery blowery deleted the update/phone-validation branch November 20, 2019 13:39
@blowery blowery removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 20, 2019
@akirk
Copy link
Member

akirk commented Nov 20, 2019

Sorry I am late but tests pass fine and I didn't see problems in the UI, so let's went.

@blowery
Copy link
Contributor

blowery commented Nov 20, 2019

Thanks, @akirk! It looked good on my end too.

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

Successfully merging this pull request may close these issues.

Vietnam phone numbers not validating for 2FA
8 participants