Framework: Move localStorage polyfill to separate bundle

[aduth]

As discovered in #7038 and #6558 before it, our local storage polyfill loading behavior is very fragile, and I suspect we're doomed to repeat it in its current state. As such, this pull request seeks to separate the polyfill into a separate bundle to be included in the page prior to the main application bundle.

Testing instructions:

Verify that your primary browser and Safari Private Mode load Calypso without any errors in the console.

Open questions:

• Are there desktop app considerations to be made here?
• Why does the polyfill bundle include vendor code? See related updating to version 1.8.10 causes new error in CommonsChunkPlugin webpack/webpack#1016 and examples/two-explicit-vendor-chunks for reference
• very important: do we need systems to whitelist public/polyfill.js since it’s a static asset?

/cc @gwwar (fixed previous breakage) @blowery (tinkerer of Webpack)

Test live: https://calypso.live/?branch=add/polyfill-bundle

[gwwar]

It feels like we should stub, or restore this value when the suite is done.

[gwwar]

This line is causing the failed tests: https://github.com/Automattic/wp-calypso/blob/master/client/lib/react-test-env-setup/index.js#L31

[gwwar]

cc @mkaz for the desktop question

[aduth]

This line is causing the failed tests:

Indeed, though interestingly different failures when updating this line. We might instead need to change lib/local-storage back into exporting a function, though this would require additional logic in the server rendered markup to invoke. I'll plan to poke at this more tomorrow.

[aduth]

Rebased and introduced a separate lib/polyfill module to better allow lib/local-storage to be used outside the context of the browser (e.g. test env setup) and more easily tested in isolation. Having a separate lib/polyfill module better aligns with "polyfill" as a bundle, rather than specifically to the localStorage polyfill (in case, for example, we want to include more polyfills, which hopefully is not the case 😄 ).

[gwwar]

Whoops lost track of this one. I can test again after a rebase

[aduth]

Thanks @gwwar , pushed a rebase to resolve merge conflicts.

I noticed that the conflicts were caused by the introduction of the ES6 function wrappers in #6117, which feels kinda similar in purpose. Do you think it would make sense to pull that into the "polyfill" bundle here? To be fair, those are more development mode utilities than polyfills. I dunno, maybe best to leave for a separate pull request in any case.

[gwwar]

Do you think it would make sense to pull that into the "polyfill" bundle here?

Let's leave that for another PR

[gwwar]

With a completely empty lib/local-storage/index.js file the polyfill chunk is still 385 kB in size in a development environment. Do we expect this amount of size overhead from webpack in the first chunk? cc @blowery

[gwwar]

👍 I confirmed that browsing with private Safari browser does not lead to errors in Calypso on this branch.

The polyfill chunk size seems unreasonably large though, so we may want to investigate if an alternative webpack configuration can give us a lighter result.

[blowery]

@gwwar hmmm.. if it's an entry chunk, it'll include the loader bits from webpack, but 385 seems... crazy. What all is in there?

[aduth]

When building as production, it appears to be much smaller - Around 30kb without any minification or gzip (~5.5kb after). Inspecting generated bundle, most of this size is CoreJS polyfills for ES6 e.g. Object.assign.

[gwwar]

30kb sounds much more reasonable. I'd be comfortable with merging this.

[gwwar]

Checking in on this one, was there anything else that needed additional testing?

[aduth]

Checking in on this one, was there anything else that needed additional testing?

No, just haven't had a chance to circle back around to this. Will need to rebase and retest to account for changes in #8161.

[aduth]

Rebased to account for #8161. @blowery : Do you think this should be included in the vendor bundle? Currently it's separate.

Also, I'm not sure why it was needed previously, but I did not include the CommonsChunkPlugin updates as in the previous version (9036fff) and it still appears to work well. In fact, the development bundle size is much more reasonable, so that could explain why it was so large before.

[blowery]

I find this a little confusing. This is an IIFE that's exporting nothing. Could it just be an IIFE in the body of the module and explicitly export null or undefined instead? like

( () => { /*do the thing */ } )();

export default null;

or maybe even don't export anything?

[blowery]

Hrm, just realized I may not understand how modules evaluate as well as I thought. Does code in the body of the module run before the module is required? Or just once on the first require?

[aduth]

I think you're right that we don't need or want the export default. The only other reason to keep it an IIFE is to allow the early return, since there can't be top-level returns in a module. Otherwise we could just put all the browser-specific polyfills inside the if block.

Or just once on the first require?

This is the intended behavior.

[aduth]

Removed IIFE export in ec93319.

[blowery]

re

Also, I'm not sure why it was needed previously, but I did not include the CommonsChunkPlugin updates as in the previous version (9036fff) and it still appears to work well. In fact, the development bundle size is much more reasonable, so that could explain why it was so large before.

Could you walk through that a bit more? I'm curious why the dev bundle would change in size at all.

[aduth]

@blowery To be honest, I don't recall why I felt the need to include it in CommonsChunkPlugin in the first place. Probably because I was referencing the approach for vendor. When rebasing, I "forgot" to include it again, yet it ran without issues and the size of the bundle was appropriate in development, so... ¯_(ツ)_/¯

[lancewillett]

@aduth Want to keep this one active?

We plan close it soon as it is an older pull with no recent activity.

[aduth]

@lancewillett Yes, I'd still like to merge this in. It's reviewable in its current state as far as I can recall.

[gwwar]

I'd be fine with 🚢'ing this after a rebase and another spot check.

[aduth]

Meh, static file has a number of downsides. Haven't decided whether they outweigh the need to create a separate config for such a small file though.

[blowery]

Yeah. I'd consider just inlining it honestly.

[aduth]

That's a good thought. In which case it acts more as an extension to what we have already for BrowseHappy?

[blowery]

That sounds cool.

[aduth]

My reluctance at this point stems from wariness to add more unminified JavaScript to the base index.jade file. The BrowseHappy stuff is already pretty non-ideal.

i'd probably bump this out to a separate webpack build entirely. This will be affected by things like the vendor dll / vendor chunk, commons plugins, etc.

Can you explain what you mean by this being affected @blowery ?

[blowery]

Just that it'll have to load after the manifest if its part of our normal webpack build. That's probably ok?

[gziolo]

My question is why we don't migrate existing code to use localForage instead of localStorage?

[aduth]

My question is why we don't migrate existing code to use localForage instead of localStorage?

We should, but it's non-trivial since the leap from synchronous to asynchronous is not always well-defined. Since it's not obvious, also noting that most usage won't be uncovered from a search of localStorage but instead the store dependency we use to interface.

[gziolo]

I did some research and it looks like localforage supports callbacks and ES6 Promises:

• https://github.com/mozilla/around/blob/90805796a0612460b18d2b5b99fd4d625bb83f08/www/js/vendor/backbone.localforage.js#L28
• https://github.com/mozilla/around/blob/90805796a0612460b18d2b5b99fd4d625bb83f08/www/js/views/users.coffee#L32

They mention callback and promises also here.

[gziolo]

I did even more research and it looks like we would need sync wrapper for localforage that would work as drop-in replacement for store :D

I think your PR makes much more sense in this context.

[matticbot]

@aduth This PR needs a rebase

[blowery]

yeah, there's a few places using it:

$ ag "require\( 'store'" -c && ag "from 'store'" -c
boot/index.js:1
layout/community-translator/launcher.jsx:1
layout/nux-welcome/index.js:1
lib/application-passwords-data/index.js:1
lib/desktop/index.js:1
lib/local-list/index.js:1
lib/plugins/store.js:1
lib/plugins/wporg-data/list-store.js:1
lib/posts/actions.js:1
lib/preferences/actions.js:1
lib/products-list/index.js:1
lib/signup/progress-store.js:1
lib/signup/flow-controller.js:1
lib/trophies-data/index.js:1
lib/user/user.js:1
my-sites/welcome/README.md:1
auth/controller.js:1
blocks/app-promo/index.jsx:1
layout/community-translator/invitation-utils.js:1
layout/masterbar/notifications.jsx:1
lib/abtest/index.js:1
lib/countries-list/index.js:1
lib/oauth-token/index.js:1
lib/olark/index.js:1
lib/sites-list/list.js:1
lib/user/support-user-interop.js:1
lib/wp/handlers/guest-sandbox-ticket.js:1
my-sites/invites/controller.js:1
my-sites/invites/invite-accept-logged-out/index.jsx:1
my-sites/pages/helpers.js:1
reader/sidebar/index.jsx:1

[gziolo]

@aduth I rebased it with master.

@fabianapsimoes Can you test this branch with the scenario described in #6555 (comment)?

[klimeryk]

With #13748 merged, this can probably be closed - unless we want to pursue this as the ultimate fix? Although, IMHO, #13748 seems solid enough (though, I'm probably biased ;)).

[aduth]

@klimeryk Sounds alright to me! We can always revisit if needed.