Framework: Enable babel-polyfill so login form works on older browsers #25419

jblz · 2018-06-11T17:10:32Z

Without these polyfills in place, UC Browser Mini (& perhaps other browsers) is not working in Calypso (seemingly) at all.

React doesn't pick up client-side to actually make the page interactive, so all the client shows is what's rendered server-side.

Taking a look at the JavaScript Environment Requirements page:

React 16 depends on the collection types Map and Set. If you support older browsers and devices which may not yet provide these natively (e.g. IE < 11) or which have non-compliant implementations (e.g. IE 11), consider including a global polyfill in your bundled application, such as core-js or babel-polyfill.

This change does precisely that.

At /log-in

Before:

Form fields are disabled and never become interactive:

After:

Form fields become enabled and functional after calypso boots:

TO TEST

Test on all "supported" browsers (Chrome, Firefox, Safari, Edge, etc.)

Everything should "still work" ;)

Test on older browsers

My setup is as follows:

Android Studio 3.1
Virtual Device with Android 4.4 (Google APIs) (Use Tools | AVD Manager to configure one for yourself)
Run the AVD with the ability to modify your hosts file (Ref)
Push a hosts file like the following:

127.0.0.1               localhost
192.168.0.111           calypso.dev
1.2.3.4           public-api.wordpress.com wordpress.com

(Where 192.168.0.111 is your local machine's "real" IP address on your local network (or some other address the emulator can access where your calypso server is running & 1.2.3.4 is your WordPress.com sandbox)

Download the Android UC Browser Mini apk from the official site.
Install the apk: adb install ~/Downloads/UCBrowser_V10.9.0.946_android_pf139_\(Build1805171001\).apk
Run this branch
In the android emulator:
- Open UC Mini
- Dismiss the "splash" list of features
- Disable Speed Mode (so you can access a local IP address)
  - Tap the 3 horizontal line button ("hamburger menu")
  - Tap Speed Mode
- Browse to http://calypso.dev:3000/log-in

IMPORTANT NOTE: In order to actually log in, you'll need to add http://calypso.dev:3000 to the list of allowed origins in the endpoint. Ping me if you need help with this part.

matticbot · 2018-06-11T17:10:33Z

Test live: https://calypso.live/?branch=fix/uc-browser-no-boot-take2

blowery · 2018-06-11T17:12:49Z

.babelrc.js

@@ -43,7 +44,7 @@ const config = {
 			{ async: isCalypsoClient && codeSplit },
 		],
 		'@babel/plugin-proposal-export-default-from',
-		'@babel/transform-runtime',
+		[ '@babel/transform-runtime', { regenerator: false } ],


I'm pretty sure we can drop this completely if we're using the polyfill

Seems happy without, so pushed the change here.

jblz · 2018-06-11T17:11:57Z

.babelrc.js

@@ -19,6 +19,7 @@ const config = {
 					browsers: [ 'last 2 versions', 'Safari >= 10', 'iOS >= 10', 'ie >= 11' ],
 				},
 				exclude: [ 'transform-classes', 'transform-template-literals' ], // transform-classes is added manually later.
+				useBuiltIns: 'entry',


The other option here is usage.

Which might improve bundle size.

usage won't help because Set and Map will only get polyfilled if they get used in our code. Since we need the polyfill for code in node_modules that's not getting transpiled, it would be prone to breakage.

IE it would work when some of our code that needs set or map runs before react does.

I tried taking out the useBuiltIns line entirely and it still works to allow log in. Should we take it out?

nah, leave it in. Right now it won't make a huge diff, but down the road if and when we start doing different builds for new and old browsers, it will.

babel-preset-env rewrites it based on the target browsers to only include the polyfills that are necessary for that set.

jblz · 2018-06-11T17:13:52Z

.babelrc.js

@@ -43,7 +44,7 @@ const config = {
 			{ async: isCalypsoClient && codeSplit },
 		],
 		'@babel/plugin-proposal-export-default-from',
-		'@babel/transform-runtime',
+		[ '@babel/transform-runtime', { regenerator: false } ],


I'm not entirely sure this is necessary, but it's included since @blowery mentioned something about disabling the regenerator transform & I wanted to make sure to capture that suggestion.

jblz · 2018-06-11T17:55:20Z

After log in, most things work well until they don't.

After clicking around a bit, I get a bunch of Uncaught TypeError: Illegal invocation in the console and the browser whitescreens on pretty much any screen.

Something else to chase here.

jblz · 2018-06-11T17:58:56Z

The Illegal invocation error is coming from the webpack-build-monitor component and I'm not really sure how to interpret it, but it seems like it's overflowing some stack and causing React to error out:

blowery · 2018-06-11T19:19:59Z

@jblz that's likely an old weirdness with console. On some browsers, you have to call the console methods with the console as the context. Patch in 944acae.

jblz · 2018-06-11T20:01:09Z

Thanks for the update, @blowery. Solves the whitescreen.

I had independently suppressed the webpack-build-monitor component from mounting & found a problem with the post normalizer's usage of DOMParser.prototype.parseFromString.

Addressed that in 4ef3980.

I'm not in love with that workaround, but needed to do something to work around:
new DOMParser().parseFromString( '<div>lol</div>', 'text/html' ) === null
😢

blowery · 2018-06-11T20:16:34Z

@jblz should we land this sooner rather than later just to get login working?

blowery · 2018-06-11T20:16:53Z

cc @jsnajdr too

DanReyLop · 2018-06-11T20:25:46Z

package.json

@@ -21,7 +21,6 @@
    "@babel/core": "7.0.0-beta.44",
    "@babel/plugin-proposal-export-default-from": "7.0.0-beta.44",
    "@babel/plugin-syntax-jsx": "7.0.0-beta.44",
-    "@babel/plugin-transform-runtime": "7.0.0-beta.44",


Please remove it from npm-shrinkwrap.json too.

blowery · 2018-06-11T20:28:35Z

I'm not in love with that workaround

@jblz that's what it was a long time ago. Should be ok.

DanReyLop · 2018-06-11T20:32:38Z

Should we delete this module now? https://github.com/Automattic/wp-calypso/tree/01dccfae03148121ff7820ef417929a12c64aec4/client/lib/wrap-es6-functions

Or maybe cleanup after this login issue is solved?

jblz · 2018-06-11T20:35:07Z

I added one more commit (c6a2239) that cleaned up a few other usages of DOMParser.prototype.parseFromString.

The only one I witnessed causing crashes was in the comments/utils module, but added the others since they'd probably do the same if executed.

Happy to split anything off that folks would like.

I'm about to:

Rebase to get the node upgrade
Update the shrinkwrap
EDIT: & fix the CI tests I broke :P

jblz · 2018-06-11T21:22:41Z

Thanks for taking a look, @DanReyLop!

Shrinkwrap updated.

Should we delete this module now? https://github.com/Automattic/wp-calypso/tree/01dccfae03148121ff7820ef417929a12c64aec4/client/lib/wrap-es6-functions
Or maybe cleanup after this login issue is solved?

I'll defer to @blowery on that one -- I have no idea :)

jblz · 2018-06-11T21:33:38Z

Fixing: ReferenceError: document is not defined

blowery · 2018-06-12T00:40:02Z

I'll defer to @blowery on that one -- I have no idea :)

Yup, we can remove it now.

blowery · 2018-06-12T00:42:06Z

client/lib/plugins/sanitize-section-content.js

@@ -129,11 +134,8 @@ const replacementFor = node => {
 * @returns {string} sanitized HTML
 */
 export const sanitizeSectionContent = content => {
-	const parser = new DOMParser();
-	const doc = parser.parseFromString( content, 'text/html' );


this change might be dangerous. DOMParser won't pull and run script tags included in the parsed DOM, while innerHTML probably will... Needs testing.

Try parsing some HTML that includes a script tag and see if it runs.

I injected a script tag and wasn't able to trigger it on a plugin page, but since I'm not 100% sure it won't make for an unsafe situation, I'll go ahead and pull it out.

jblz · 2018-06-12T20:57:50Z

@mattwondra Can you please give this a run through with regards to the Directly integration? I tested on a modern browser and nothing jumped out at me, but I'm not entirely sure what to test otherwise.

If you're not the best person to test this & know who is, can you please let me know who?

Thanks in advance!

jblz · 2018-06-12T21:06:47Z

@blowery @DanReyLop -- I pulled out the wrap-es6-functions lib & usage if you'd like to take another look.

I also reverted the DOMParser usage in client/lib/plugins/sanitize-section-content in abundance of caution. I'll see about testing that more fully in a follow-up.

Does anyone see any potential issues with other places innerHTML would be called on non-compliant browsers? Is there another approach we could use to mitigate them?

blowery · 2018-06-14T14:14:38Z

This should be fine and won't affect Directly at all — it's a third-party widget so its code doesn't get compiled within Calypso

The change is polyfilling various things across the entire execution environment. Unless Directly is running purely inside an iframe, it could be affected. I'm not super familiar with how Directly works...

mattwondra · 2018-06-14T17:16:47Z

Unless Directly is running purely inside an iframe, it could be affected.

Great point, thanks. The vast majority of the code does run in an iframe, but there's a small script we're loading in the main execution environment that provides a thin handler into Directly (and loads the iframe).

I've tested Directly myself without any issue, so I think we're in the clear.

blowery

LGTM

blowery · 2018-06-14T17:49:41Z

Hrm, quite a bit of + movement in the delivered bundles. http://iscalypsofastyet.com/branch?branch=fix/uc-browser-no-boot-take2

Looking into it. Think we just have to eat it though.

blowery · 2018-06-14T18:15:03Z

Ok, most of the additions are from the babel helpers no longer being inlined. We might be able to bring back babel-transform-runtime and only transform the helpers..

This reverts commit dc30264.

blowery · 2018-06-14T18:49:12Z

Nice, mostly red size changes now. +28k zipped on vendor~build though. We can hopefully bring that down in the future by doing a separate build for modern browsers.

jblz · 2018-06-14T19:17:05Z

Still working well for me in the Android 4.4 emulator running UC Mini & in Chrome on my Mac.

Thanks for the fining, @blowery.

sirreal · 2018-06-15T09:29:44Z

It would be great to publish a wider notice about this! The polyfill change is significant and impacts a lot of devs.

jblz · 2018-06-15T15:00:44Z

@sirreal Agreed! p4TIVU-8YW-p2

jblz added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Login labels Jun 11, 2018

jblz requested a review from blowery June 11, 2018 17:10

blowery reviewed Jun 11, 2018

View reviewed changes

jblz commented Jun 11, 2018

View reviewed changes

DanReyLop reviewed Jun 11, 2018

View reviewed changes

jblz force-pushed the fix/uc-browser-no-boot-take2 branch from c6a2239 to 3422658 Compare June 11, 2018 21:20

jblz 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 Jun 11, 2018

jblz added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jun 11, 2018

blowery reviewed Jun 12, 2018

View reviewed changes

jblz force-pushed the fix/uc-browser-no-boot-take2 branch from 0c94570 to 3b863f4 Compare June 12, 2018 20:55

jblz requested a review from mattwondra June 12, 2018 20:56

jblz force-pushed the fix/uc-browser-no-boot-take2 branch from 36e09ac to 71e7f52 Compare June 14, 2018 03:12

update shrinkwrap

7244a06

blowery 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 Jun 14, 2018

blowery approved these changes Jun 14, 2018

View reviewed changes

blowery added 3 commits June 14, 2018 14:18

Revert "Remove @babel/transform-runtime"

a9a1673

This reverts commit dc30264.

add back the babel-transform-runtime and transform helpers

5353b77

add back minification

27ff72a

blowery force-pushed the fix/uc-browser-no-boot-take2 branch from 7865cbd to 27ff72a Compare June 14, 2018 18:46

blowery requested a review from jsnajdr June 14, 2018 18:49

blowery added the [Status] Needs e2e Testing label Jun 14, 2018

jblz merged commit 65c8abe into master Jun 14, 2018

jblz deleted the fix/uc-browser-no-boot-take2 branch June 14, 2018 20:36

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 14, 2018

blowery removed the [Status] Needs e2e Testing label Jun 15, 2018

gwwar mentioned this pull request Jun 15, 2018

Framework: Investigate using babel-polyfill #2360

Closed

dmsnell added [Type] Bug [Type] Enhancement labels Jun 17, 2018

jsnajdr mentioned this pull request Jul 11, 2018

Avoid using path.skip() in the Babel transform for action type inlining #25961

Merged

ockham mentioned this pull request Jul 16, 2018

Framework: Move @babel/polyfill import to webpack config #26094

Merged

This was referenced Aug 23, 2018

Add more manual renewal reasons for immediate login, and tweak the immediate login text #26855

Merged

Docs: Update javascript doc to allow Array methods #26856

Merged

Framework: Enable babel-polyfill so login form works on older browsers #25419

Framework: Enable babel-polyfill so login form works on older browsers #25419

Conversation

jblz commented Jun 11, 2018 • edited Loading

TO TEST

Test on all "supported" browsers (Chrome, Firefox, Safari, Edge, etc.)

Test on older browsers

matticbot commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blowery Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jblz commented Jun 11, 2018

jblz commented Jun 11, 2018

blowery commented Jun 11, 2018 • edited Loading

jblz commented Jun 11, 2018 • edited Loading

blowery commented Jun 11, 2018 • edited Loading

blowery commented Jun 11, 2018

Choose a reason for hiding this comment

blowery commented Jun 11, 2018

DanReyLop commented Jun 11, 2018

jblz commented Jun 11, 2018 • edited Loading

jblz commented Jun 11, 2018

jblz commented Jun 11, 2018

blowery commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jblz commented Jun 12, 2018

jblz commented Jun 12, 2018

blowery commented Jun 14, 2018

mattwondra commented Jun 14, 2018

blowery left a comment

Choose a reason for hiding this comment

blowery commented Jun 14, 2018

blowery commented Jun 14, 2018

blowery commented Jun 14, 2018

jblz commented Jun 14, 2018

sirreal commented Jun 15, 2018

jblz commented Jun 15, 2018

jblz commented Jun 11, 2018 •

edited

Loading

blowery Jun 11, 2018 •

edited

Loading

blowery commented Jun 11, 2018 •

edited

Loading

jblz commented Jun 11, 2018 •

edited

Loading

blowery commented Jun 11, 2018 •

edited

Loading

jblz commented Jun 11, 2018 •

edited

Loading