Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 5 commits into from
Mar 14, 2017
Merged

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

merged 5 commits into from
Mar 14, 2017

Conversation

samouri
Copy link
Contributor

@samouri samouri commented Mar 6, 2017 •
edited by gziolo
Loading

We recently launched a change to the build system: #11352 that breaks wp-desktop.

It made it so that instead of generating a bundle per environment, we only generate a single build. T
here are a couple changes to make here and to wp-desktop to make it compatible.
The changes necessary for wp-calypso are:

  1. send over config in desktop.jade
  2. switch to node style export/import for config. We cannot use es6 modules for config because it gets pulled in by wp-desktop's node server
  3. a recent commit added in an assignment to server/page/index's req.context which doesn't exist in wp-desktop (not sure why).
  4. remove config from sections.js. I'm not sure why this is necessary just yet
  5. Rename client/lib/config to client/lib/create-config to avoid naming collision with Desktop files.

Testing

  1. make sure that regular wp-calypso functionality isn't broken at all. still boots, etc:
  • make run
  • make test
  1. Load up wp-desktop and tune its claypso to this branch. then change wp-desktop branch to: Framework: updated makefile to work with single build.js wp-desktop#275. if both these branches are checked out then it should build successfully

@matticbot
Copy link
Contributor

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

@samouri samouri changed the base branch from master to try/framework/ssr-config March 6, 2017 13:41
@matticbot matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 6, 2017
@samouri samouri mentioned this pull request Mar 6, 2017
Merged
@matticbot matticbot added [Size] S Small sized issue and removed [Size] XL Probably needs to be broken down into multiple smaller issues labels Mar 6, 2017
@samouri samouri mentioned this pull request Mar 6, 2017
Merged
4 tasks
gziolo
gziolo reviewed Mar 7, 2017
View reviewed changes
client/sections.js Outdated
@@ -7,8 +7,7 @@ const path = require( 'path' );
/**
* Internal dependencies
*/
const config = require( 'config' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is a good idea, i may put config back. I think i removed it while i was looking into why config was breaking everything

gziolo
gziolo reviewed Mar 7, 2017
View reviewed changes
client/config/index.js Outdated
@@ -1,7 +1,8 @@
/**
* Internal dependencies
*/
import createConfig from 'lib/config';
//import createConfig from 'lib/config';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should remove comments and proceed without ES6 modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely

@samouri samouri force-pushed the try/framework/ssr-config-desktop-compat branch from f039dcd to 3483909 Compare March 13, 2017 21:55
@matticbot matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] S Small sized issue labels Mar 13, 2017
@samouri samouri changed the base branch from try/framework/ssr-config to master March 13, 2017 21:57
@samouri samouri force-pushed the try/framework/ssr-config-desktop-compat branch from 3483909 to 0bc75c2 Compare March 13, 2017 22:15
@matticbot matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 13, 2017
@samouri samouri force-pushed the try/framework/ssr-config-desktop-compat branch from 0bc75c2 to 8307eb6 Compare March 13, 2017 22:17
@matticbot matticbot added [Size] S Small sized issue and removed [Size] XL Probably needs to be broken down into multiple smaller issues labels Mar 13, 2017
@samouri 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 Mar 14, 2017
@samouri
Copy link
Contributor Author

samouri commented Mar 14, 2017 •
edited
Loading

To test:

@matticbot matticbot added the [Size] S Small sized issue label Mar 14, 2017
@gziolo gziolo force-pushed the try/framework/ssr-config-desktop-compat branch from 28077fb to 42f2431 Compare March 14, 2017 12:56
@matticbot matticbot added the [Size] S Small sized issue label Mar 14, 2017
@gziolo
Copy link
Member

gziolo commented Mar 14, 2017

I decided to rename client/lib/config to avoid naming collisions with Desktop.

@samouri
Copy link
Contributor Author

samouri commented Mar 14, 2017

I decided to rename client/lib/config to avoid naming collisions with Desktop.

Sort of feels like we are running away from a problem, but heck its simpler 👍 . create-config is also a better name for the module

@samouri samouri added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 14, 2017
@samouri samouri merged commit a9c9900 into master Mar 14, 2017
@samouri samouri deleted the try/framework/ssr-config-desktop-compat branch March 14, 2017 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gziolo gziolo gziolo left review comments

Assignees
No one assigned
Labels
[Size] S Small sized issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@samouri @matticbot @gziolo