enable user-bootstrapping when secrets are present by samouri · Pull Request #20267 · Automattic/wp-calypso

samouri · 2017-11-27T23:02:29Z

User bootstrapping was disabled in staging, horizon, and wpcalypso in #17849 -- this was intended to be a test and then if successful it would also be disabled in production. However, issues with localization were discovered in staging.

This PR re-enables user-bootstrapping. Since it was never disabled in production, this PR should have no effect to users but should ideally unblock React 16.

Next Steps

merge r16 upgrade Framework: update to react 16 (take three) #19723
merge fix for detecting user location: Forward geo location header in user bootstrap requests #18971

matticbot · 2017-11-27T23:02:30Z

Test live: https://calypso.live/?branch=config/re-enable-bootstrap

samouri · 2017-11-27T23:37:29Z

currently getting an error in docker:

TypeError: Cannot read property 'style.css' of undefined
    at eval (eval at wrap (/calypso/node_modules/pug-runtime/wrap.js:6:10), <anonymous>:3:1419)
    at template (eval at wrap (/calypso/node_modules/pug-runtime/wrap.js:6:10), <anonymous>:3:2136)
    at Object.exports.renderFile (/calypso/node_modules/pug/lib/index.js:428:38)
    at Object.exports.renderFile (/calypso/node_modules/pug/lib/index.js:418:21)
    at View.exports.__express [as engine] (/calypso/node_modules/pug/lib/index.js:465:11)
    at View.render (/calypso/node_modules/express/lib/view.js:126:8)
    at tryRender (/calypso/node_modules/express/lib/application.js:639:10)
    at Function.render (/calypso/node_modules/express/lib/application.js:591:3)
    at ServerResponse.render (/calypso/node_modules/express/lib/response.js:961:7)
    at serverRenderError (/calypso/build/webpack:/server/render/index.js:152:7)

ockham · 2017-11-27T23:53:28Z

Maybe this: https://github.com/Automattic/wp-calypso/blob/master/server/pages/index.pug#L79

And urls should be defined in server/pages/index.js IIRC...

ockham · 2017-11-28T00:23:34Z

Hmm, seems to wfm.

yoavf · 2017-11-28T07:22:46Z

Sorry for letting this hang for so long. Completely fell off my radar.
Seeing the same error as @samouri right now.

samouri · 2017-11-28T08:57:32Z

I was able to diagnose the issue to being caused by a lack of secrets.json. Also we don't set request.context until immediately after the user bootstrap so the 500 page didn't have access to context.urls which explains the cryptic and seemingly unrelated error.

Lets continue the discussion of how to handle this in slack instead of GH.

…GeoIP. (#17849)" This reverts commit e3bca1e.

ockham · 2017-11-28T10:49:03Z

Can we merge this without the changes to server/pages? IIUC, that fixes the broken 500 error, which folks are seeing because of missing secrets.json.

IMO, server/pages has seen too many one-off fixes lately that didn't take all possible consequences into account; before making any more changes, I'd rather start by writing some unit tests to define what behavior we want to see. It has also harder and harder to reason about. (Sry if I'm being over-cautious, but changing next( error) to next() already makes a big difference...)

samouri · 2017-11-28T17:56:53Z

Can we merge this without the changes to server/pages? IIUC, that fixes the broken 500 error, which folks are seeing because of missing secrets.json.

yes and no :).

The broken 500 error was fixed just by moving the context higher up which is pretty safe.
the conversion from next( error ) --> next() is unrelated and doesn't solve anything and I'll remove it.

That said, there still is one more fix that needs to go in here. Namely that the server still crashes when secrets.json is missing but it should just continue on with user-bootstrapping disabled. I think i've tracked it down to a recent modification: 257546f#diff-f400d1ccc42dff5b55b97ac4a9970205 where a throw was added to an async function that never ends up invoking its callback.

samouri · 2017-11-28T19:23:44Z

Even though this is a small PR, there is a lot of background info needed to understand its motivation and how to test. I'll try to break down some of that here.

First off, Calypso uses server-side-rendering to render certain pages like /themes to non-logged in users. #17849 disabled user bootstrapping. User bootstrapping is necessary on the server side to determine if a user is logged in or not -- so when this was disabled the server always assumes that the user is not logged in and will always ssr isomorphic routes. This results in an incorrect flash on the screen for isomorphic routes but nothing breaking...at least not until React 16 modified the client side hydration implementation. This is why the attempt#2 to upgrade to R16 botched in staging (but would have worked in production).

So the solution seems straightforward right?
Lets just re-enable use-bootstrapping, then the server can determine when to ssr properly, and then React 16 will be happy. Not so fast!

We want local docker development to be easy to setup. After re-enabling user-bootstrapping for all stages post development, the server started crashing if a secrets.json was missing. It also wasn't properly logging the right errors and was silently dropping all requests because of this recent change. The last piece of this solution was to disable wpcom-user-bootstrapping when secrets.json is missing so that developers don't need special keys to test docker.

changelog:

move the req.context = getDefaultContext to be above the user bootstrapping request so that if errors occur within user bootstrapping then 500 page will have access to the context object
modify the config parser to automatically disable wpcom-user-bootstrapping if the real secrets file is missing. it also logs this now.
modify the user bootstrap script to invoke its callback with an error instead of throwing an error
re-enable user-boostrapping in the configs for all stages except development

testing instructions

There are two situations that need to be tested. one is when secrets.json is present, and the other is when it is missing.

with secrets.json

local dev should work fine.
docker build should also behave as expected. you should not see an error message saying that user-bootstrap is disabled.
when logged out, ssr should work
when logged in, '/themes' should not flash the logged out page

without secrets.json

local dev should work fine
docker build should also build properly except when run you will see the message that user-boostrapping has been disabled
when logged out ssr should work
when logged in /themes will incorrectly flash the logged out view. this is expected and okay in this case

ockham · 2017-11-28T19:34:01Z

server/config/parser.js

+		console.error( 'Disabling server-side user-bootstrapping because of a missing secrets.json' );
+		data.features[ 'wpcom-user-bootstrap' ] = false;
+	}
+


Hmm, the config parser is a place where I wouldn't expect this.
Would a viable alternative be to add these checks explicitly where we're now checking for config.isEnabled( 'wpcom-user-bootstrap' )?

i agree it isn't ideal. the client doesn't have access to the secrets though :/.
we can make it more generic in: 'disableSecretsRequiredFeatures' but lets leave that till there are more

Hmm, I've noticed client/ side checks for config.isEnabled( 'wpcom-user-bootstrap' ) now, but are those even necessary? We should be able to simply check if the server is giving us a window.currentUser object in bootstrapping -- possibly having the server set it to false if bootstrapping is off...

(Fine to do in another PR I guess so we don't block the upgrade any longer.)

We should be able to simply check if the server is giving us a window.currentUser

thats a good idea! and would clean this up.

(Fine to do in another PR I guess so we don't block the upgrade any longer.)

that would be my vote. I'll make an issue for it now. I'm wary of diving down more rabbit holes right now in case that turns out to be more complicated than we expect. I also wouldn't want to add even more fixes onto this pr.

issue: #20308

ockham · 2017-11-28T19:35:02Z

server/config/test/parser.js

+		expect( serverData.features[ 'wpcom-user-bootstrap' ] ).toBe( false );
+		expect( clientData.features[ 'wpcom-user-bootstrap' ] ).toBe( false );
+		expect( errorSpy ).toHaveBeenCalledTimes( 1 );
+	} );


Yay for tests tho 🎉

ockham · 2017-11-28T19:45:34Z

@samouri I just pushed e922fa4 to get rid of the temporary context variable in setUpLoggedInRoute altogether, hoping to make it a little less confusing. Feel free to discard (but if you do, please consider at least dropping that req.context = context;, as found at the very bottom of the commit's diff 😄).

ockham · 2017-11-28T19:50:13Z

server/user-bootstrap/index.js


 		if ( typeof API_KEY !== 'string' ) {
-			throw new Error( 'Unable to boostrap user because of invalid API key in secrets.json' );
+			callback( new Error( 'Unable to boostrap user because of invalid API key in secrets.json' ) );


Much better 😄

samouri · 2017-11-28T20:34:22Z

@ockham: thanks for the commit! I like it 👍

dmsnell · 2017-11-29T00:58:12Z

I'm not sure how to really test this besides just trying it out. I'm not familiar enough with the code but am under the impression that things are "already broken" in production.

My recommendation would thus be to plan and announce a deployment where we ask everyone to do a few things after deploying to quickly confirm that there aren't any surprises we didn't anticipate.

dmsnell

@samouri I did not test this but in Slack you told me that you did (following your instructions)

I'm not confident in my ability to verify the code changes or your testing instructions, but it feels like this should be a good change and I see no problems with the code.

If we were voting I'd say let's just merge, test, deploy, and test this in the morning on some non-Friday non-weekend day soon and ask everyone who's around to do some basic normal testing in case this change has consequences that aren't obvious.

samouri · 2017-11-29T15:47:04Z

Ha! My scheme worked. I've successfully tricked @sirreal into thoroughly testing the functionality introduced here in the React 16 PR!!
See #19723 (review)

I'll ship this within the hour ⭐️

samouri self-assigned this Nov 27, 2017

samouri added the [Status] In Progress label Nov 27, 2017

samouri requested review from DanReyLop, blowery, ockham and yoavf November 27, 2017 23:03

samouri mentioned this pull request Nov 27, 2017

Framework: Don't serve SSR content to logged in folks #19611

Closed

samouri added 2 commits November 28, 2017 01:14

Revert "Disable "wpcom-user-bootstrap" until it works correctly with …

c10a7b6

…GeoIP. (#17849)" This reverts commit e3bca1e.

fix for local docker wpcalypso

15ca372

samouri force-pushed the config/re-enable-bootstrap branch from c7ec448 to 15ca372 Compare November 28, 2017 09:14

bluefuton added the Framework label Nov 28, 2017

samouri added 2 commits November 28, 2017 10:04

fix error callback for non-existing secret

253ff55

diabled user-bootstrapping when there is no valid secrets.json

747bdf2

samouri requested a review from stephanethomas November 28, 2017 19:25

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 Nov 28, 2017

ockham reviewed Nov 28, 2017

View reviewed changes

Drop context variable from setUpLoggedInRoute()

e922fa4

ockham reviewed Nov 28, 2017

View reviewed changes

This was referenced Nov 28, 2017

remove checks to wpcom-user-bootstrap in client #20308

Closed

Framework: update to react 16 (take three) #19723

Merged

samouri changed the title ~~Revert "Disable "wpcom-user-bootstrap" until it works correctly with …~~ enable user-bootstrapping when secrets are present Nov 28, 2017

dmsnell approved these changes Nov 29, 2017

View reviewed changes

samouri merged commit d8862c9 into master Nov 29, 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 Nov 29, 2017

samouri deleted the config/re-enable-bootstrap branch November 29, 2017 16:39

This was referenced Dec 7, 2017

Themes: links to /themes/themename do not work #20423

Closed

Themes: links to /design do not work #20416

Closed

Conversation

samouri commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Next Steps

Uh oh!

matticbot commented Nov 27, 2017

Uh oh!

samouri commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ockham commented Nov 27, 2017

Uh oh!

ockham commented Nov 28, 2017

Uh oh!

yoavf commented Nov 28, 2017

Uh oh!

samouri commented Nov 28, 2017

Uh oh!

ockham commented Nov 28, 2017

Uh oh!

samouri commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samouri commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

testing instructions

Uh oh!

ockham Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

samouri Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ockham Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

ockham Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

samouri Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

samouri Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

ockham Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

ockham commented Nov 28, 2017

Uh oh!

ockham Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

samouri commented Nov 28, 2017

Uh oh!

dmsnell commented Nov 29, 2017

Uh oh!

dmsnell left a comment

Choose a reason for hiding this comment

Uh oh!

samouri commented Nov 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

samouri commented Nov 27, 2017 •

edited

Loading

samouri commented Nov 27, 2017 •

edited

Loading

samouri commented Nov 28, 2017 •

edited

Loading

samouri commented Nov 28, 2017 •

edited

Loading

samouri Nov 28, 2017 •

edited

Loading