Framework: ES Modules + Tree Shaking #16057

samouri · 2017-07-10T20:57:48Z

Webpack2 and Webpack 3 introduced a couple major features:

native es6 modules + tree shaking. This promises to make our modules smaller, compile times faster (less babel transpilation), and our code more correct (greater compile time checks).
Scope Hoisting. This feature is about making the code smaller and the runtime properties faster by removing wepback wrapper funcs in favor of direct inlining when possible.

Currently, we cant take advantage of either because we are still transpiling to commonjs modules in our babel configuration.

In this PR I try to find (and hopefully solve) all of the blockers towards disabling commonjs transpilation and enabling tree-shaking/scope hoisting.

To Test

Test everything on: https://dserve.a8c.com/?branch=try/scope-hoisting.

matticbot · 2017-07-10T20:57:58Z

Test live: https://calypso.live/?branch=try/scope-hoisting

samouri · 2017-10-08T23:45:11Z

After running the commonjs-exports codemod on client and reverting some files used by the server, webpack throws hundreds of errors! They look like this:

WARNING in ./client/boot/common.js
160:50-65 "export 'isDefaultLocale' was not found in 'lib/i18n-utils'

The reason that is an error has to do with a problem that is relatively pervasive in wp-calypso and it has to do with trying to destructure imports.

Destructuring Imports
In proper es6 you cannot destructing an import, you can only import named exports via the "destructing" notation. That means if you write:
import {thing1, thing2} from 'lib' then thing1 and thing2 must be actual named exports and not just keys in a default exported object.

So for a file that looks like this:

import { thing1, thing2 } from 'lib';

This would a syntactically correct lib file:

// correct lib implementation
export const thing1 = function(){ ... };
export const thing2 = function(){ ... };

This is an incorrect one:

// incorrect lib implementation
export default {
  thing1: function(){ ... },
  thing2: function(){ ... }
};

Here is a list i generated of offending files and the frequency with which their exports are being used incorrectly. Note all relative urls need more investigation.

  11 '../'
   2 '../comment-replies-cache'
   3 './assembler'
   1 './comment-replies-cache'
  66 './constants'
   5 './controller'
  10 './data'
   1 './helpers'
   3 './schema'
   1 './services'
  15 './utils'
   1 'components/phone-input/data'
   1 'components/post-schedule/utils'
  46 'config'
  31 'lib/analytics'
  10 'lib/analytics/ad-tracking'
 101 'lib/cart-values'
  12 'lib/cart-values/cart-items'
   1 'lib/cart/store/cart-analytics'
   3 'lib/countries-list'
   5 'lib/credit-card-details'
   2 'lib/data-poller'
   1 'lib/domains/assembler'
  58 'lib/domains/constants'
   3 'lib/domains/email-forwarding'
   2 'lib/domains/google-apps-users'
   4 'lib/domains/whois/utils'
   1 'lib/feature-detection'
   5 'lib/feed-post-store/actions'
   3 'lib/feed-post-store/constants'
   1 'lib/feed-post-store/display-types'
   3 'lib/feed-stream-store/constants'
 112 'lib/formatting'
  18 'lib/i18n-utils'
   2 'lib/impure-lodash'
   2 'lib/like-store/actions'
   1 'lib/media-serialization'
   1 'lib/media/store'
  10 'lib/media/utils'
  32 'lib/paths'
   1 'lib/plugins/utils'
  12 'lib/posts/utils'
 208 'lib/products-values'
  17 'lib/route'
  23 'lib/route/path'
   3 'lib/shortcode'
   1 'lib/signup/actions'
   2 'lib/signup/step-actions'
  18 'lib/site/utils'
   4 'lib/sites-list/actions'
   5 'lib/store-transactions'
   3 'lib/touch-detect'
  38 'lib/upgrades/actions'
  73 'lib/url'
   4 'lib/url/support'
   1 'lib/users/actions'
  24 'lib/viewport'
   6 'me/event-recorder'
   4 'me/purchases/paths'
  54 'my-sites/controller'
  22 'my-sites/domains/paths'
   4 'my-sites/invites/utils'
   2 'signup/config/flows'
   1 'signup/utils'
   3 'state/action-types'
   8 'state/current-user/constants'
  19 'state/jetpack-connect/actions'
  20 'state/jetpack-connect/selectors'
   2 'state/jetpack-sync/actions'
   3 'state/plugins/premium/actions'
  18 'state/plugins/premium/selectors'
   5 'state/plugins/wporg/actions'
   7 'state/plugins/wporg/selectors'
  55 'woocommerce/lib/nav-utils'

samouri · 2017-10-08T23:45:39Z

A full list of all the bad-imports.txt

samouri · 2017-10-11T00:46:49Z

next two things:

try out named-exports codemod
fix leftovers by hand
profit?

samouri · 2017-10-11T06:58:37Z

found an issue in notifications-panel. I create a PR there and until its merged will need to test with:

npm install Automattic/notifications-panel#fix/imports-for-tree-shaking

samouri · 2017-10-11T09:49:00Z

Really cool news about the stricter es6 syntax: the code will be able to verify a greater amount of correctness at compile time.

Now the interesting bit, I found 8 7 issues that are most likely bugs in production right now:

WARNING in ./client/state/notices/middleware.js
214:1724-1752 "export 'PUBLICIZE_CONNECTION_REFRESH' was not found in 'state/action-types'
fix: publicize: remove unused middleware code #18839
WARNING in ./client/state/login/magic-login/reducer.js
23:8-25 "export 'INTERSTITIAL_PAGE' was not found in './constants'
fix: Magic Logic: interstitial page constant missing #18840
WARNING in ./client/state/signup/optional-dependencies/reducer.js
14:4-27 "export 'suggestedUsernameSchema' was not found in './schema'
fix: Signup: fix suggestedUsernameSchema #18841
WARNING in ./client/state/site-settings/exporter/reducers.js
97:7-36 "export 'EXPORT_ADVANCED_SETTINGS_FAIL' was not found in 'state/action-types'
fix: Site Setting Exporter: fix action-name in reducer #18842
WARNING in ./client/state/sharing/publicize/publicize-actions/reducer.js
61:21-43 "export 'publicizeActionsSchema' was not found in './schema'
fix: Publicize Actions: fix typo in the schema #18843
WARNING in ./client/reader/tag-stream/index.js
17:55-70 "export 'recommendedTags' was not found in './controller'
fix: Reader Tags: remove recs #18844
WARNING in ./client/my-sites/sharing/connections/services-group.jsx
44:20-45 "export 'hasOwnProperty' (imported as 'Components') was not found in './services'
NOTE: this last one isn't a bug. it just won't be legal in es6land
fix: MySites Sharing: give services a shimmed hasOwnProperty #18845

samouri · 2017-10-11T09:59:37Z

A few more notes from me whacking at the issue today:

we need to get rid of all requires and use imports instead. that has ramifications in a few areas:
1.1 - [x] sections loader need to use import
1.2 - [x] extensions-loader (reducers) needs to use import
1.3 anywhere we are relying upon dead-code elimination and nested requires we will need a different strategy. one idea could be: export default ( env === 'production' ? noop : actualexport at relevant location instead of at the import site but its less natural and harder to track down which are dependent on env. I'll do research to see if theres a better way.
- change required in notifications-panel

current status:

At the start of this PR (after running codemods) there were over 1k compile time errors. Now I'm down to what I think are tens of runtime ones. I'm pretty excited about the progress here so far! The notifications-panel loads properly and I believe the rest of the errors will all be due to the extra requires

DanReyLop · 2017-10-11T11:12:59Z

1.3 anywhere we are relying upon dead-code elimination and nested requires we will need a different strategy. one idea could be: export default ( env === 'production' ? noop : actualexport at relevant location instead of at the import site but its less natural and harder to track down which are dependent on env. I'll do research to see if theres a better way.

Relevant: webpack/webpack#2061

If I understood it correctly, Webpack does a basic pass to check if an import is really used, and then UglifyJS does the actual code elimination. So, here:

import { otherFunc1, otherFunc2 } from './otherModule';

function callOther() {
  otherFunc1();
  otherFunc2();
}

export default function () {
  if (always_false) {
    callOther();
  }
}

Webpack will assume that otherFunc1 and otherFunc2 are being used. UglifyJS then will remove the if and the callOther function entirely, but will keep the import.

But in this case:

import { otherFunc1, otherFunc2 } from './otherModule';

export default function () {
  if (always_false) {
    otherFunc1();
    otherFunc2();
  }
}

Webpack correctly detects that otherFunc1 and otherFunc2 aren't being used, so trees will be shaken and the import will be removed entirely.

I think our use cases can be adapted to the second case, just use the imported module inside a constant conditional and it will be optimized away.

For the first case, I assume that if the imported module is part of the same bundle, and scope hoisting is fully functioning, then everything can be optimized away by UglifyJS too, but I have nothing solid to back this up.

samouri · 2018-01-22T22:48:20Z

I've created a wp-desktop fix, but its pretty hacky: Automattic/wp-desktop#406.

It may be worthwhile to look into whether or not we can selectively include add-module-exports for jest and then remove it from babelrc

jsnajdr · 2018-01-22T23:25:53Z

client/boot/common.js

@@ -22,6 +22,9 @@ import { setLocale, setLocaleRawData } from 'state/ui/language/actions';
 import { setCurrentUserOnReduxStore } from 'lib/redux-helpers';
 import { installPerfmonPageHandlers } from 'lib/perfmon';
 import { getSections, setupRoutes } from 'sections-middleware';
+import { checkFormHandler } from 'lib/protect-form';
+import notices from 'notices';
+import authController from 'auth/controller';


Hah, the notices and authController modules still use export default = { ... } :)

just double checking: this comment is just an observation and doesn't mean that i should make a change here right?

Yes, it can wait for a separate PR.

jsnajdr · 2018-01-22T23:37:50Z

client/lib/happychat/connection-ng.js

@@ -23,7 +24,7 @@ import {
 	requestTranscript,
 } from 'state/happychat/connection/actions';

-const debug = require( 'debug' )( 'calypso:happychat:connection' );
+const debug = debugFactory()( 'calypso:happychat:connection' );


This should be debugFactory( 'calypso:happychat:connection' ). Otherwise you create a debug logger and log a message immediately :)

whoops! Nice catch. I'll fixem

jsnajdr · 2018-01-22T23:38:02Z

client/lib/happychat/connection.js

@@ -23,7 +24,7 @@ import {
 	requestTranscript,
 } from 'state/happychat/connection/actions';

-const debug = require( 'debug' )( 'calypso:happychat:connection' );
+const debug = debugFactory()( 'calypso:happychat:connection' );


jsnajdr · 2018-01-22T23:48:16Z

There's a require( 'wpcom-proxy-request' ) in client/lib/wpcom-undocumented/lib/me.js that could also be updated to import.

dmsnell · 2018-01-23T01:06:05Z

@jsnajdr et. al. @samouri and I had a conversation in Slack concerning the change in semantics from runtime require() and module-init-time import statements. In most cases it won't matter (and I don't know right now if it matters for wpcom-proxy-request but my guess is that it does) but there are reasons to leave it as require() until we have import() running.

Namely the import statements load and initialize their referenced modules as soon as the calling module initializes. While the code may end up in the bundle either way, there are places where we can't execute that code because of conditions which aren't met in some runtime circumstances. In something like wpcom-proxy-request it may not matter but if we don't need it then we have no reason to initialize that module anyway.

jsnajdr · 2018-01-23T08:51:19Z

I don't know right now if it matters for wpcom-proxy-request but my guess is that it does

The wpcom-proxy-request module does very little static initialization: it sets a few variables to false or null, computes origin from window.location and sends a probe request to window.postMessage to determine if it supports non-string messages. All the heavy lifting, i.e., initializing the proxy iframe, is done lazily on first call to request.

But you have a very good point: for the modules that this PR changes from conditional imports to static, we should check if any of them does some expensive static initialization.

If it does, I think the most reliable remedy is to rewrite these modules to do the expensive initialization lazily -- only when an exported function is called or exported class instantiated for the first time.

jsnajdr · 2018-01-23T16:28:37Z

I found an insidious bug that's exposed by enabling tree shaking and scope hoisting: in lib/posts/utils.js, there is isPublished function that internally calls this.isBackDatedPublished, but there is no this binding.

It used to work when the isPublished and isBackDatedPublished functions were properties of one exports object, but now they are defined as standalone functions in the global scope.

To experience the runtime error, go to the post editor page at /post/:site_id/:post_id.

How many other modules are affected and where to find them?

samouri · 2018-01-23T17:31:12Z

It used to work when the isPublished and isBackDatedPublished functions were properties of one exports object, but now they are defined as standalone functions in the global scope.

So the solution should be to remove the this right?

jsnajdr · 2018-01-23T17:39:38Z

So the solution should be to remove the this right?

Yes.

samouri · 2018-01-23T17:43:48Z

How many other modules are affected and where to find them?

Specifically for the "this" bug you've found, we can do a grep for "this" for all *.js (non jsx) files.
This should at least give us a sense for how often this practice is done in Calypso.

The result of the query was 17 files. They were grouped like this:

bin/*.js: 9 scripts within bin/. They each reference "this" within comments. These are safe.
client/lib/happychat: 2 cases. Both of them are because these files actually use classes. This is pretty unusual since most of our classes live in jsx files. These two should also be safe.
client/lib/post: 1 case. This is the bug you found.
sections-preload.js, wordpress-com.js, inline-imports.js: uses the word this in a comment. safe.
webpack configs: both use the word this a couple of times, also safe.

It seems to me like you caught something pretty rare. Technically its possible this also happens within jsx files, but I wouldn't know how to search for that. @dmsnell knows of some structural grep tools that could help.

It is worrisome to think that there still may be other slight inconsistencies between how CJS and esmodules behave in webpack. I don't think it should stop us progressing with this PR though.

dmsnell · 2018-01-23T17:58:37Z

sends a probe request to window.postMessage

that's the kind of thing we need to be careful about because if we initialize that via import when it was previously trapped behind a window check then we'll throw an exception out of the browser environment (server-renderings etc…).

Insidious bug…It used to work when the isPublished and isBackDatedPublished functions were properties of one exports object, but now they are defined as standalone functions in the global scope.

Good catch! This isn't inherently a problem though; it was just missed in the conversion (probably by me). We rewrote many functions to go from this.something or Something.somethingElse to non-prefixed something()s. We just need to update the code.

From what I've seen we have very few cases (outside of React components) where we need this or rely on its behavior (that is, where we couldn't just as well rewrite it without this).

structural grep tools that could help

maybe? these might be hard to find in the JSX files. on the other hand, if it's in a JSX file it's probably safe because this wouldn't extend outside of the library. my guess from a practical standpoint is that if there are any more of these out there they will surface and we will fix them.

It is worrisome to think that there still may be other slight inconsistencies

very much so. this is also why I think in this case code-modding would have been disastrous in practice.

I don't think it should stop us progressing with this PR though.

👍

samouri · 2018-01-23T20:19:10Z

timmyc: I know I'm referencing code from about 2 years ago, but I was wondering if you remember what the intention of getPagePath() was? I made a modification in the latest commit to make it pass the tests and want to make sure I'm not changing behavior in a way that would cause trouble: c2404e9

samouri · 2018-01-23T20:21:27Z

Actually, its only used in a single line in the whole app: client/post-editor/post-editor.jsx.

timmyc · 2018-01-24T00:49:51Z

Memory lane right there, well it was originally used for displaying the correct base path for post slugs. Your changes seem fine to me.

jsnajdr

I tested this PR quite thoroughly, clicking around Calypso everywhere I could, and I didn't find any bugs. Let's 🚢 this.

The getPagePath function still needs some fixing, but that's a trivial change.

@dmsnell has a good point that the way how we load wpcom-proxy-request and other modules can be a little dangerous and could easily lead to errors like dereferencing window in Node environment.

But I don't think that anything is broken now -- it's more like the code is easy to break if we are not careful. We should clean up the imports a little bit soon, but it's not a blocker for this PR.

jsnajdr · 2018-01-24T17:21:16Z

client/lib/posts/utils.js

-
-	return pathParts.join( '/' );
+	const postUrl = get( post, [ 'other_URLs', 'permalink_URL' ] ) || post.URL;
+	return removeSlug( postUrl );


... and you can also revert this back to just return removeSlug( post.URL ) -- the permalink case was already handled.

While I think thats true, it'll fail one of the unit tests.

edit: i lied. it works fine now that the return was added back :)

This reverts commit 2f082a9.

samouri added the [Status] In Progress label Jul 10, 2017

samouri self-assigned this Jul 10, 2017

matticbot added the [Size] S Small sized issue label Jul 10, 2017

samouri added this to the Calypso Settings: Performance (m5) milestone Jul 10, 2017

samouri added Build Framework labels Jul 10, 2017

tyxla removed this from the Calypso Settings: Performance (m5) milestone Jul 11, 2017

samouri force-pushed the try/scope-hoisting branch from ec7c705 to 44c18bd Compare July 15, 2017 15:38

matticbot added [Size] M Medium sized issue and removed [Size] S Small sized issue labels Jul 15, 2017

samouri force-pushed the try/scope-hoisting branch from 84bfa3a to a68cba7 Compare July 19, 2017 17:01

matticbot added [Size] L Large sized issue and removed [Size] M Medium sized issue labels Jul 19, 2017

samouri mentioned this pull request Jul 19, 2017

Build: Remove Custom Bundler #16379

Closed

6 tasks

samouri force-pushed the try/scope-hoisting branch from a68cba7 to 6de0e92 Compare September 11, 2017 22:14

samouri mentioned this pull request Sep 25, 2017

Build: Remove custom bundler and extract minification #18074

Merged

samouri force-pushed the try/scope-hoisting branch from 6de0e92 to 36aa82e Compare October 8, 2017 23:01

samouri mentioned this pull request Oct 9, 2017

apply codemod: commonjs-exports #18678

Merged

samouri force-pushed the try/scope-hoisting branch from 96ab0cb to e0834f5 Compare October 10, 2017 23:43

samouri force-pushed the try/scope-hoisting branch from 546d91b to f3cab96 Compare October 11, 2017 02:13

samouri mentioned this pull request Oct 11, 2017

Modify exports to make webpack es6 happy Automattic/notifications-panel#184

Merged

samouri force-pushed the try/scope-hoisting branch from 528c1af to 50fa7a8 Compare October 11, 2017 07:48

jsnajdr reviewed Jan 22, 2018

View reviewed changes

debugFactory fix

9fcca0c

address 'this' issue within client/lib/posts

7332b03

samouri mentioned this pull request Jan 23, 2018

Framework/no implicit import paths #20142

Closed

fix getPagePath

c2404e9

samouri requested a review from coderkevin January 24, 2018 06:13

jsnajdr approved these changes Jan 24, 2018

View reviewed changes

:headbandage: :headbang: thanks @jsnajdr

7897423

jsnajdr reviewed Jan 24, 2018

View reviewed changes

samouri mentioned this pull request Jan 24, 2018

Build: enable scope hoisting #21798

Merged

simplify getPagePath

819a9ce

samouri merged commit 2f082a9 into master Jan 24, 2018

samouri added a commit that referenced this pull request Jan 24, 2018

Revert "Framework: ES Modules + Tree Shaking (#16057)"

c562dac

This reverts commit 2f082a9.

samouri added a commit that referenced this pull request Jan 24, 2018

Revert "Framework: ES Modules + Tree Shaking (#16057)" (#21800)

39eabde

This reverts commit 2f082a9.

alisterscott deleted the try/scope-hoisting branch February 1, 2018 06:39

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework: ES Modules + Tree Shaking #16057

Framework: ES Modules + Tree Shaking #16057

samouri commented Jul 10, 2017 •

edited

Loading

matticbot commented Jul 10, 2017

samouri commented Oct 8, 2017 •

edited

Loading

samouri commented Oct 8, 2017 •

edited

Loading

samouri commented Oct 11, 2017

samouri commented Oct 11, 2017 •

edited

Loading

samouri commented Oct 11, 2017 •

edited

Loading

samouri commented Oct 11, 2017 •

edited

Loading

DanReyLop commented Oct 11, 2017

samouri commented Jan 22, 2018 •

edited

Loading

jsnajdr Jan 22, 2018

samouri Jan 23, 2018 •

edited

Loading

jsnajdr Jan 23, 2018

jsnajdr Jan 22, 2018

samouri Jan 23, 2018

jsnajdr Jan 22, 2018

jsnajdr commented Jan 22, 2018

dmsnell commented Jan 23, 2018

jsnajdr commented Jan 23, 2018

jsnajdr commented Jan 23, 2018

samouri commented Jan 23, 2018

jsnajdr commented Jan 23, 2018

samouri commented Jan 23, 2018

dmsnell commented Jan 23, 2018

samouri commented Jan 23, 2018 •

edited

Loading

samouri commented Jan 23, 2018

timmyc commented Jan 24, 2018

jsnajdr left a comment

jsnajdr Jan 24, 2018

samouri Jan 24, 2018

samouri Jan 24, 2018

Framework: ES Modules + Tree Shaking #16057

Framework: ES Modules + Tree Shaking #16057

Conversation

samouri commented Jul 10, 2017 • edited Loading

To Test

matticbot commented Jul 10, 2017

samouri commented Oct 8, 2017 • edited Loading

samouri commented Oct 8, 2017 • edited Loading

samouri commented Oct 11, 2017

samouri commented Oct 11, 2017 • edited Loading

samouri commented Oct 11, 2017 • edited Loading

samouri commented Oct 11, 2017 • edited Loading

DanReyLop commented Oct 11, 2017

samouri commented Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

samouri Jan 23, 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

jsnajdr commented Jan 22, 2018

dmsnell commented Jan 23, 2018

jsnajdr commented Jan 23, 2018

jsnajdr commented Jan 23, 2018

samouri commented Jan 23, 2018

jsnajdr commented Jan 23, 2018

samouri commented Jan 23, 2018

dmsnell commented Jan 23, 2018

samouri commented Jan 23, 2018 • edited Loading

samouri commented Jan 23, 2018

timmyc commented Jan 24, 2018

jsnajdr left a 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

samouri commented Jul 10, 2017 •

edited

Loading

samouri commented Oct 8, 2017 •

edited

Loading

samouri commented Oct 8, 2017 •

edited

Loading

samouri commented Oct 11, 2017 •

edited

Loading

samouri commented Oct 11, 2017 •

edited

Loading

samouri commented Oct 11, 2017 •

edited

Loading

samouri commented Jan 22, 2018 •

edited

Loading

samouri Jan 23, 2018 •

edited

Loading

samouri commented Jan 23, 2018 •

edited

Loading