SSR The config module #11352

samouri · 2017-02-10T23:13:05Z

Calypso currently has different configuration for its different stages and the way that we handle it causes some inefficiencies. We have almost the same exact modules for wpcalypso, horizon, staging, and production. The only difference between them is the config module. This single module difference essentially forces us to separate out bundles for each environment and call the bundler 4 times within a docker build.

This PR seeks to move the config module to be sent by the server as part of the initial payload which:

cuts build times in a down because we don't need to rebuild docker image 4 times (and have 4 different versions of the bundler: bundle-wpcalypso.js, bundle-production.js, etc.
No longer need to generate a config before running unit tests

Speed tests on my machine:

Building the docker image took 4:44.62 in this branch /try/framework/ssr-config
11:44.25 on master

To Test

start calypso using make run
run all the unit tests (and also circle ci)
Open https://calypso.live/?branch=try/framework/ssr-config and make sure it still works
build the docker image. then run it in the 4 different environments and make sure it works as expected

related issues this should solve as well:

Generate config at runtime: Build: Generate config at runtime, rather than build time #10608
Always use the right config during tests: Testing: Make sure we always use test env config when running client tests #5856

matticbot · 2017-02-10T23:13:06Z

Test live: https://calypso.live/?branch=try/framework/ssr-config

samouri · 2017-02-11T08:36:28Z

client/config/index.js

@@ -0,0 +1,11 @@
+let config;
+


todo: add comment or readme explaining whats going on

gziolo · 2017-02-11T17:15:07Z

Cool idea! Thanks for working on it. It would be great to improve config handling, in particular in the context of unit tests.

I'll double check this proposal on Monday. There might be security concern with this approach, because at least on desktop there are secret keys loaded in the server build. This code would probably expose those keys to the client. It needs to be verified.

dmsnell · 2017-02-13T21:54:17Z

client/config/index.js

+	config.isEnabled = window.isEnabled;
+} else {
+	throw new Error( 'config not available' );
+}


we can flip this conditional and then use a constant value…

/** * Manages config flags for various deployment builds * @module config/index */ if ( 'undefined' === typeof window || ! window.config ) { throw new ReferenceError( `No configuration was found: please see client/config/README.md for more information` ); } /** * @typedef {Object} CalypsoConfig * @property {Function} anyEnabled whether one of a set of properties is enabled * @property {Function} isEnabled whether a specific property is enabled */ export default config = { ...window.config, anyEnabled: window.anyEnabled, isEnabled: window.isEnabled, };

Another thing to think about that may or may not come into play here is that Object.assign() sets values while destructuring assignment defines them. These are small semantic differences and if we have any properties defined on config then we may want to make sure we carry those over (and not run or evaluate them while copying).

http://www.2ality.com/2016/10/rest-spread-properties.html

Thanks for the docs :). I'm definitely going to put those in. Also TIL about ReferenceError!

Another thing to think about that may or may not come into play here is that Object.assign() sets values while destructuring assignment defines them.

window.config is a function here. I don't think spread works for those...does it? I usually prefer spread/object literal when possible.

absolutely - I didn't realize/think about that it was a function. that's too bad, but thanks for verifying that and adjusting accordingly

dmsnell

@samouri thanks for making this change. If we can get this ready to go it's going to be a major boon for development.

I'm not that familiar with this part of the build process so I don't have much to offer. However, I would definitely implore you to spend some time understanding the scope of the improvements you are bringing and the system they change and then write up some guides for the benefit of others (README.md is fair enough)

This would significantly help further efforts to improve the build process.

👍

alisterscott · 2017-02-14T00:28:26Z

I spun this up locally in docker and ran some e2e tests against it, couldn't find any issues.
Noticed it was faster than usually spinning up in Docker, well done.

ehg · 2017-02-14T12:51:52Z

One problem with this that comes to mind is that we won't have the correct build time config for each environment. For example, when building the sections we use config all over the place: https://github.com/Automattic/wp-calypso/blob/master/client/wordpress-com.js#L160

samouri · 2017-02-14T15:18:56Z

i think @ehg is right, I can't access devdocs in the docker instance for some reason even though config.isEnabled('devdocs') --> true

I'm testing out putting CALYPSO_ENV env var before the $(BUNDLER) call in the makefile

EDIT: its this line https://github.com/Automattic/wp-calypso/pull/11352/files#diff-3254677a7917c6c01f55212f86c57fbfR42

Since it builds with CALYPSO_ENV=production, the right bundle (devdocs) never gets generated. This just got more complex 🤔

samouri · 2017-02-14T17:21:55Z

A solution i came up with that I think may be pretty valid is:

Always make all the bundles.

The client can request which bundles it needs. There is one big constraint that this idea would create though:

We cannot expect/make a bundle contain different things for different environments. I think this is reasonable. If you want code to be different/only run in certain envs, it needs to be in its own bundle

dmsnell · 2017-02-14T21:47:07Z

The client can request which bundles it needs. There is one big constraint that this idea would create though:

We cannot expect/make a bundle contain different things for different environments. I think this is reasonable. If you want code to be different/only run in certain envs, it needs to be in its own bundle

related is existing work on async loaders and changing the way we request bundles. if we had a more flexible and robust system for loading code asynchronously this would be less of a concern I would imagine since we wouldn't be tied to the sections.

by the way, the section bundles have been serving us incredibly well and props to those who made them work the way they do!

gziolo · 2017-02-16T06:39:15Z

Dockerfile

@@ -39,11 +39,7 @@ RUN     npm install --production || npm install --production
 COPY     . /calypso

 # Build javascript bundles for each environment and change ownership


The comment above becomes no longer valid.

gziolo · 2017-02-16T06:41:09Z

Dockerfile

-          CALYPSO_ENV=stage make build-stage && \
-          CALYPSO_ENV=production make build-production && \
-          chown -R nobody /calypso
+RUN	CALYPSO_ENV=production make build && chown -R nobody /calypso


This need to be further investigated, because we have 3 ways of detecting env in client and server code:

config( 'env )

NODE_ENV

CALYPSO_ENV

gziolo · 2017-02-16T06:46:00Z

client/sections.js

-		secondary: true,
-		enableLoggedOut: true
-	} );
+sections.push( {


I agree that we need to remove those feature flag checks from files where sections definition are located. At the same time I think we should move existing logic to entry point of each individual section. This means I'm suggesting to bring back those file guard to the first file which isn't generated by Webpack, to make sure we don't change any existing logic. We can remove some flags in separate PRs.

i think both are required in this PR:

removing feature flag checks for build (so that all bundles are generated)

adding feature flag checks to controller files (so that routes inaccessible in a specific env stay inaccessible)

Does this mean we'll be generating a bundle for devdocs regardless of config settings?

@mtias yes, the bundle will be generated regardless of config (this should be bringing us to a single build)

I'm not sure generating these bundles if they are disabled at the config level is a good path. It makes extending Calypso and using it to build things harder.

How does it differ from using feature flags nested deeper into a chunk? I think it is a very good simplification that bring lots of benefits simplifying build process and drastically reducing its time.

However we need to sync it with extensions. I double checked it and it turns out that our code depends on env_id (https://github.com/Automattic/wp-calypso/blob/master/client/sections.js#L17). We plan to use production env for all non-development envs when preparing build targeted for Docker, which means it won't work as expected for staging, wpcalypso and horizon.

@mtias: I'm not sure how generating /not-generating bundles can affect extensibility.
Whether or not the client requests the bundle is up to it. For example, theres no reason the devdocs bundle can't exist on the server for NODE_ENV=production, as long as the client doesn't register the /devdocs routes via page

@gziolo I'll fix that now. My current solution will be to move that check to the individual index.js and hopefully eventually we can autowrap the extensions's default exports with an env_id check

I think I found solution with 3d98ba8 to this issue. This allows to bundle all sections and extensions, but at the same time never load chunks that are hidden behind the feature flag using env_id in package.json file.

@mtias @mcsf @ehg Let us know if that solution would work :)

gziolo · 2017-02-16T07:06:17Z

Just wanted to summarize what we need to be aware of when working on this PR.

Calypso code depends on the following constructs:

As far as I understand there was intent to have:

config( 'env_id' ) === CALYPSO_ENV // staging, horizon, development, production, desktop ...
config( 'env' ) === NODE_ENV // production or development

This is how I read it from config files.

All of those different expressions are compared in many places against development and production mostly, but you can also find references to desktop and other envs. In the context of having one docker build based on CALYPSO_ENV=production make build all code checks that target all non production envs are in question.

We also need to keep in mind that we are using here Webpack DefinePlugin which is able to replace all code occurrences like process.env.NODE_ENV !== 'production' into boolean value. If any such check inside if statement is replaced with false Weppack is going to remove also all code that is living inside this control statement. We need to keep in mind that in this case NODE_ENV is derived from CALYPSO_ENV which makes reasoning even more complicated. It's even difficult to explain what can break when we generate build using production CALYPSO_ENV, but run it with staging, but I can imagine there might slip in some subtle changes to the codebase. I guess it isn't a big deal, but we should process such cases and maybe find one common way of accessing current env :)

Much better in code explanation how process.env.NODE_ENV works with Webpack can be found here:

// Webpack can optimize bundles if it can detect that a block will
// never be reached. Since NODE_ENV is defined using DefinePlugin,
// these debugging modules will be excluded from the production build.

I hope this helps reviewing this PR :)

gziolo · 2017-02-16T07:34:16Z

One more important note. We need to make sure we don't break Desktop app introducing those changes. I don't remember how config was consumed in https://github.com/Automattic/wp-desktop but I have a feeling that it could have it's own flavors :)

Tug · 2017-02-17T00:22:37Z

server/config/index.js

+	'<%= anyEnabled %>'
+) ( {
+	data: JSON.stringify( clientData ),
+	config: config.toString(),


I wouldn't worry about duplicating the config module on the client if I were you, I think we should avoid serializing functions here and depending on several globals.

1 global object containing only the raw configuration should be enough for our needs:

window.configData = <%= data %>;

Moreover, we could easily share the config module between the client and the server.

function loadConfig( configData = null ) { data = configData || window && window.configData; }

Agree on this one. At the moment we have this code inlined in HTML file:

function config(key) { if (key in data) { return data[key]; } throw new Error('config key `' + key + '` does not exist'); }function isEnabled(feature) { return !!data.features[feature]; }function anyEnabled() { var args = Array.prototype.slice.call(arguments); return args.some(function (feature) { return isEnabled(feature); }); }

We should rather keep config API code in the JS file.

I just tested this out locally. While I'd rather rely on the same module in both client/server, I couldn't figure out how to ensure webpack would exclude the serverConfig from the bundle (without changing what this file exports)

I branched off a PR to test this out: #11454

@Tug @samouri I think I managed to refactor it to use shared config API. See a80d2dc.

I decided to use createConfig higher-order function to make it testable. Let me know if this works for you.

Tug · 2017-02-17T00:23:45Z

server/bundler/bin/bundler.js

@@ -114,7 +114,7 @@ webpack( webpackConfig, function( error, stats ) {
 	} ).map( function( chunk ) {
 		return path.join( process.cwd(), 'public', chunk.file );
 	} );
-	files.push( path.join( process.cwd(), 'public', 'vendor.js' ) );
-
+	files.unshift( path.join( process.cwd(), 'public', 'vendor.js' ) );


We usually rebase to avoid those merge commits ;)

git rebase origin/master

this may not have been the best place to test out the resolve conflicts feature of GitHub. Is there anything different (or worse) about this method?

one at the front, one at the end…

Tug · 2017-02-17T00:29:53Z

Looks good so far, good job on this @samouri :)
I had a minor comment to slightly improve the config module. Also added a few missing feature checks on the client.

I think this might need extensive testing before we are confident about a merge but I'm optimist, don't hesitate to keep pinging people to have a look!

gziolo · 2017-02-17T06:52:37Z

client/wordpress-com.js

-		secondary: true
-	} );
-}
+sections.push( {


@scottsweb It looks like you created scaffolding for VIP section. It doesn't look like it is actively developed. Do you use those pages or would it be good idea to remove those files?

We are not currently using those files and any further development there is at least 3 months out. If you would like to remove them then please do. I imagine a great deal has changed in Calypso since they were put together :) cc @gziolo

Thanks for letting me know. I'll open a follow up PR once this one gets merged ;)

@withinboredom

…plate literals 1. use es6 template literal instead of lodash template 2. remove anyEnabled from config api since it is never used 3. CALYPSO_ENV --> config( 'env_id' ) 4. docs changes thanks @withinboredom / slight doc updates 5. move extensions check to individual extension instead of in sections.js so it happens at runtime

…erences Build: Move env check for extensions to bundler loader

…forEach

…, combine server/config with client/lib/config

…rdpress-com section building

samouri · 2017-03-06T12:36:16Z

client/wordpress-com.js

@@ -157,7 +150,7 @@ const sections = [
 		name: 'themes',
 		paths: [ '/design' ],
 		module: 'my-sites/themes',
-		enableLoggedOut: config.isEnabled( 'manage/themes/logged-out' ),
+		enableLoggedOut: true,


Since manage/themes/logged-out is true in every environment configuration I think that modifying this should be harmless. Is this true?

cc @seear

Correct. We should also remove manage/themes/logged-out from the config files if we set this to true, in case anyone tries to turn that flag off in the future expecting it to have any effect.

Darn, I missed this comment earlier. I'll follow up with a PR after this lands

samouri · 2017-03-06T12:38:11Z

client/wordpress-com.js

 sections.push( {
 	name: 'post-editor',
-	paths: editorPaths,
+	paths: [ '/post', '/page', '/edit' ],


Since manage/custom-post-types is true in every environment configuration I think that modifying this should produce no changes

cc @aduth

Since manage/custom-post-types is true in every environment configuration I think that modifying this should produce no changes

Noticing I hadn't replied here. Yes, this should be fine, but we should probably also remove the config flag altogether if we're starting to make assumptions here that it's enabled in all environments.

gziolo · 2017-03-06T12:54:04Z

client/lib/config/test/index.js

+				];
+
+				envs.forEach( env => {
+					process.env.NODE_ENV = env;


It would be nice to revert process.env.NODE_ENV to value set before this test suite is executed.

By the way we use only two values for NODE_ENV: development and production.

I think thats what Line 54 is doing right?

yea, that whole section should be modifying CALYPSO_ENV not NODE_ENV. Its a completely separate issue to this PR though, so i'd rather fix that in a smaller future pr

I missed that, sorry :)

2nd comment is still valid though.

It's correct as it is in my opinion to use NODE_ENV. The idea here is take advantage of dead code elimination and never execute this check in non-development envs. @dmsnell can you confirm?

gziolo · 2017-03-06T13:00:52Z

client/wordpress-com.js

-		group: 'sites'
-	} );
-}
+sections.push( {


manage/drafts isn't enabled on production and staging. We need to add this check in my-sites/drafts

I looked into it, and its already there :)
https://github.com/Automattic/wp-calypso/blob/master/client/my-sites/drafts/index.js

Yes, I missed that ;)

gziolo · 2017-03-06T13:03:57Z

client/wordpress-com.js


-if ( config.isEnabled( 'reader' ) ) {


It's safe to remove. We don't have the same check inside all reader modules, but it is set to true for all envs.

samouri · 2017-03-06T15:33:49Z

There are a couple action items for after this lands:

Calypso.live compatibility
wp-desktop compatibility

Calypso.live

this should be easy and is just changing a single string from build-wpcalypso.js --> build.js

wp-desktop

There is nothing fundamental about this PR that is incompatible with wp-desktop. I spun up a branch that makes it compatible and it requires relatively small changes to both both repos. The biggest issue I ran into was when both wp-desktop requires a module that exists in both repos it currently prefers wp-calypso whereas it should probably prefer itself (/lib/config).

prototype prs: Attempt at making ssr-config compatible with wp-desktop #11785, Framework: updated makefile to work with single build.js wp-desktop#275

gziolo · 2017-03-22T20:05:58Z

server/config/regenerate-client.js

-	} ),
-	obj = {};
-
-keys.forEach( function( key ) {


@samouri I think we need to bring it back when we expose clientData in server/config/parser.js. It performs filtering based on keys loaded from client.json file.

aduth · 2017-03-31T20:12:17Z

One subtle change from this pull request is that it's no longer possible to use debug in wpcalypso, horizon, or staging environment because the Webpack configuration will now treat all non-local environments as production.

samouri · 2017-03-31T21:54:45Z

@aduth, if it is important to keep debug in other envs, we can modify that module replacement to instead make a wrapper around debug that will no-op if config( 'env_id' ) === 'production'

aduth · 2017-04-03T18:21:20Z

It's not my use-case, so I can't really speak to whether it's important. Just passing along an observed change from @jmosky who'd expected to be able to use debug logging in staging.

p1490989187543272-slack-calypso

jmosky · 2017-04-05T15:41:44Z

It's extremely useful to be able to see what events fire as I go through WP.com. It doesn't matter to me whether I have to go to https://calypso.live/?branch=master vs. just launching the debugging tool in staging. Also, I think this might be related: #12277 .

samouri · 2017-04-05T18:33:31Z

i'll whip up a pr that allows debug statements to go through in all stages except production with a runtime check to env_id
PR #12841

samouri added Framework [Status] In Progress labels Feb 10, 2017

samouri self-assigned this Feb 10, 2017

samouri force-pushed the try/framework/ssr-config branch 2 times, most recently from 1f3dc51 to 74a0d6c Compare February 11, 2017 08:12

samouri commented Feb 11, 2017

View reviewed changes

samouri mentioned this pull request Feb 11, 2017

Build: Generate config at runtime, rather than build time #10608

Closed

samouri added Build and removed Framework labels Feb 11, 2017

This was referenced Feb 11, 2017

npm pre-hook to setup config for tests #6605

Closed

Testing: Make sure we always use test env config when running client tests #5856

Closed

samouri force-pushed the try/framework/ssr-config branch from 9fa2c85 to e9c32ff Compare February 12, 2017 12:39

dmsnell reviewed Feb 13, 2017

View reviewed changes

samouri 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 Feb 15, 2017

gziolo reviewed Feb 16, 2017

View reviewed changes

Tug reviewed Feb 17, 2017

View reviewed changes

gziolo reviewed Feb 17, 2017

View reviewed changes

samouri and others added 4 commits March 6, 2017 05:07

ssr the config (pr 11352): Remove no longer necessary CALYPSO_ENV ref…

389f642

…erences Build: Move env check for extensions to bundler loader

ssr the config (pr 11352): Add in filter instead of condition within …

f54dcda

…forEach

ssr the config (pr 11352): Fix linting issues, remove extraneous test…

baae88d

…, combine server/config with client/lib/config

samouri force-pushed the try/framework/ssr-config branch from 9670e46 to baae88d Compare March 6, 2017 10:07

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 6, 2017

samouri added 2 commits March 6, 2017 05:36

ssr the config (pr 11352): make config functions curried with data

a33aa78

ssr the config (pr 11352): Remove all usages of config module from wo…

b4bfc60

…rdpress-com section building

samouri commented Mar 6, 2017

View reviewed changes

gziolo reviewed Mar 6, 2017

View reviewed changes

samouri mentioned this pull request Mar 6, 2017

Attempt at making ssr-config compatible with wp-desktop #11785

Merged

samouri merged commit b2b925c into master Mar 6, 2017

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

samouri mentioned this pull request Mar 6, 2017

look at bundle.js instead of bundle-development.js Automattic/calypso-live-branches#7

Closed

gziolo mentioned this pull request Mar 7, 2017

Framework: Remove unused VIP sites #11818

Merged

2 tasks

blowery mentioned this pull request Mar 7, 2017

Build: Move the vendor DLL to a commons chunk #11771

Closed

gziolo reviewed Mar 22, 2017

View reviewed changes

samouri deleted the try/framework/ssr-config branch March 31, 2017 21:50

samouri mentioned this pull request Apr 5, 2017

Framework: Allow debug everywhere #12841

Merged

2 tasks

aduth mentioned this pull request Apr 25, 2017

DevDocs: Switch from using Doctrine to using JSDoc with full parse #10487

Closed

		@@ -39,11 +39,7 @@ RUN npm install --production \|\| npm install --production
		COPY . /calypso

		# Build javascript bundles for each environment and change ownership

SSR The config module #11352

SSR The config module #11352

Conversation

samouri commented Feb 10, 2017 • edited Loading

To Test

matticbot commented Feb 10, 2017

Choose a reason for hiding this comment

gziolo commented Feb 11, 2017 • edited Loading

dmsnell Feb 13, 2017 • 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

dmsnell left a comment

Choose a reason for hiding this comment

alisterscott commented Feb 14, 2017

ehg commented Feb 14, 2017

samouri commented Feb 14, 2017 • edited Loading

samouri commented Feb 14, 2017 • edited Loading

dmsnell commented Feb 14, 2017

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 Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouri Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouri Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Feb 16, 2017 • edited Loading

gziolo commented Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

gziolo Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Feb 17, 2017 • 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

Tug commented Feb 17, 2017

Choose a reason for hiding this comment

scottsweb Feb 20, 2017 • 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

samouri Mar 6, 2017 • 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

samouri Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

samouri Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Mar 6, 2017 • 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

gziolo Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

samouri commented Mar 6, 2017

Calypso.live

wp-desktop

gziolo Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

aduth commented Mar 31, 2017

samouri commented Mar 31, 2017

aduth commented Apr 3, 2017

samouri commented Feb 10, 2017 •

edited

Loading

gziolo commented Feb 11, 2017 •

edited

Loading

dmsnell Feb 13, 2017 •

edited

Loading

samouri commented Feb 14, 2017 •

edited

Loading

samouri commented Feb 14, 2017 •

edited

Loading

samouri Feb 16, 2017 •

edited

Loading

samouri Feb 16, 2017 •

edited

Loading

samouri Feb 21, 2017 •

edited

Loading

gziolo commented Feb 16, 2017 •

edited

Loading

gziolo commented Feb 16, 2017 •

edited

Loading

gziolo Feb 17, 2017 •

edited

Loading

gziolo Feb 17, 2017 •

edited

Loading

scottsweb Feb 20, 2017 •

edited

Loading

samouri Mar 6, 2017 •

edited

Loading

samouri Mar 6, 2017 •

edited

Loading

samouri Mar 6, 2017 •

edited

Loading

gziolo Mar 6, 2017 •

edited

Loading

gziolo Mar 6, 2017 •

edited

Loading

gziolo Mar 22, 2017 •

edited

Loading

samouri commented Apr 5, 2017 •

edited

Loading