[WIP] Split "admin" out into it's own app-like thing #7484

ErisDS · 2016-10-04T11:27:41Z

This is a WIP based on top of #7479 - the intention is to colocate all admin-related code. At the moment, that's in a folder, and treated like an express sub-app. I'm trying to figure out if this can become a form of internal-app.

It breaks things, however f8fe928 shows what we need to fix in order to truly separate the admin client from the server.

That is - we need to 1) move the updateCheck code and 2) fix the configuration endpoints.

In this PR, I collected up the admin-related code from 3 different places:

core/server/controllers/admin.js
core/server/routes/admin.js
core/server/middleware/index.js
core/server/helpers/index.js

ALL THIS CODE!!!!! Is used to render the index.html page for the admin panel!

With this PR, it gets moved to core/server/admin.

The only thing that still lives elsewhere is the core/server/views/default.hbs file, which is actually created by ember build on the client side. DIRTY HACK ALERT! I would have moved this as well, but that would require a commit to Ghost-Admin to change where it is output.

In order to make this all work really nicely, I ideally need to move the assets which are rendered from the admin from being served at E.g. /ghost/ghost.min.js to being on a subpath of /ghost/ like ghost/assets/ghost.min.js.

This will require an update in the ghost admin utils/gh-paths.js file as well as on the server side.

Is this going to work long term when we move towards having ember create it's index.html file?

How hard is it to get ember to create it's index.html file and where will it live? Can we do that in advance, so that I can work on switching over?

core/client

@@ -1 +1 @@
-Subproject commit 0a163d733315dfee2f5a20ea6aed41ffc7bc3bda
+Subproject commit 0c2c81def806df1fa5cb0a55afff68e60883f20d


ErisDS · 2016-10-04T18:58:38Z

Ok, so I cleaned up the diablogical mess I'd made of this, so that it's just a teeny bit easier to maybe see what's changing.

I've also updated my PR message to include some questions for @kevinansfield 😉

ErisDS · 2016-10-08T16:40:33Z

Have taken the WIP tag off of this PR now. I've left the 4 commits separate for review, but the first commit has a full commit message

There are still many more TODOs, however this is complete in that the admin-code is separated from the other code, and it works independently.

I'll keep working on this and PR other commits/TODO-removals as I go

refs TryGhost#4172 - move all the admin related code from controllers, routes and helpers into a single location - refactor the way assets are registered, to keep everything working for now - duplicate & fix the checkSSL middleware to use req.originalURL - remove the decide-is-admin middleware - move the redirect-to-setup middleware into the admin - TODO: remove the ugly hack for loading assets - TODO: remove the duplicated checkSSL call - TODO: rethink the structure, this should probably be an internal app

- cleanup to help us see what needs to be done to decouple the client & server

- replace `decide-is-admin` middleware with simple `app.set()` call - update all the places which depende on this

- redirectToSetup is only needed in the admin app - this is one less confusing thing inside of core/server/middleware 🎉

@todo

closes #4172, closes #6948, refs #7491, refs #7488, refs #7542, refs #7484 * 🎨 Co-locate all admin-related code in /admin - move all the admin related code from controllers, routes and helpers into a single location - add error handling middleware explicitly to adminApp - re-order blogApp middleware to ensure the shared middleware is mounted after the adminApp - TODO: rethink the structure of /admin, this should probably be an internal app * 💄 Group global middleware together - There are only a few pieces of middleware which are "global" - These are needed for the admin, blog and api - Everything else is only needed in one or two places * ✨ Introduce a separate blogApp - create a brand-new blogApp - mount all blog/theme only middleware etc onto blogApp - mount error handling on blogApp only * 🎨 Separate error handling for HTML & API JSON - split JSON and HTML error handling into separate functions - re-introduce a way to not output the stack for certain errors - add more tests around errors & an assertion framework for checking JSON Errors - TODO: better 404 handling for static assets Rationale: The API is very different to the blog/admin panel: - It is intended to only ever serve JSON, never HTML responses - It is intended to always serve JSON Meanwhile the blog and admin panel have no need for JSON errors, when an error happens on those pages, we should serve HTML pages which are nicely formatted with the error & using the correct template * 🐛 Fix checkSSL to work for subapps - in order to make this work on a sub app we need to use the pattern `req.originalUrl || req.url` * 🔥 Get rid of decide-is-admin (part 1/2) - delete decide-is-admin & tests - add two small functions to apiApp and adminApp to set res.isAdmin - mount checkSSL on all the apps - TODO: deduplicate the calls to checkSSL by making blogApp a subApp :D - PART 2/2: finish cleaning this up by removing it from where it's not needed and giving it a more specific name Rationale: Now that we have both an adminApp and an apiApp, we can temporarily replace this weird path-matching middleware with middleware that sets res.isAdmin for api & admin * 🎨 Wire up prettyURLs on all Apps - prettyURLs is needed for all requests - it cannot be global because it has to live after asset middleware, and before routing - this does not result in duplicate redirects, but does result in duplicate checks - TODO: resolve extra middleware in stack by making blogApp a sub app * ⏱ Add debug to API setup * 🎨 Rename blogApp -> parentApp in middleware * 🎨 Co-locate all blog-related code in /blog - Move all of the blogApp code from middleware/index.js to blog/app.js - Move routes/frontend.js to blog/routes.js - Remove the routes/index.js and routes folder, this is empty now! - @todo is blog the best name for this? 🤔 - @todo sort out the big hunk of asset-related mess - @todo also separate out the concept of theme from blog * 🎉 Replace middleware index with server/app.js - The final piece of the puzzle! 🎉 🎈 🎂 - We no longer have our horrendous middleware/index.js - Instead, we have a set of app.js files, which all use a familiar pattern * 💄 Error handling fixups

@todo

closes TryGhost#4172, closes TryGhost#6948, refs TryGhost#7491, refs TryGhost#7488, refs TryGhost#7542, refs TryGhost#7484 * 🎨 Co-locate all admin-related code in /admin - move all the admin related code from controllers, routes and helpers into a single location - add error handling middleware explicitly to adminApp - re-order blogApp middleware to ensure the shared middleware is mounted after the adminApp - TODO: rethink the structure of /admin, this should probably be an internal app * 💄 Group global middleware together - There are only a few pieces of middleware which are "global" - These are needed for the admin, blog and api - Everything else is only needed in one or two places * ✨ Introduce a separate blogApp - create a brand-new blogApp - mount all blog/theme only middleware etc onto blogApp - mount error handling on blogApp only * 🎨 Separate error handling for HTML & API JSON - split JSON and HTML error handling into separate functions - re-introduce a way to not output the stack for certain errors - add more tests around errors & an assertion framework for checking JSON Errors - TODO: better 404 handling for static assets Rationale: The API is very different to the blog/admin panel: - It is intended to only ever serve JSON, never HTML responses - It is intended to always serve JSON Meanwhile the blog and admin panel have no need for JSON errors, when an error happens on those pages, we should serve HTML pages which are nicely formatted with the error & using the correct template * 🐛 Fix checkSSL to work for subapps - in order to make this work on a sub app we need to use the pattern `req.originalUrl || req.url` * 🔥 Get rid of decide-is-admin (part 1/2) - delete decide-is-admin & tests - add two small functions to apiApp and adminApp to set res.isAdmin - mount checkSSL on all the apps - TODO: deduplicate the calls to checkSSL by making blogApp a subApp :D - PART 2/2: finish cleaning this up by removing it from where it's not needed and giving it a more specific name Rationale: Now that we have both an adminApp and an apiApp, we can temporarily replace this weird path-matching middleware with middleware that sets res.isAdmin for api & admin * 🎨 Wire up prettyURLs on all Apps - prettyURLs is needed for all requests - it cannot be global because it has to live after asset middleware, and before routing - this does not result in duplicate redirects, but does result in duplicate checks - TODO: resolve extra middleware in stack by making blogApp a sub app * ⏱ Add debug to API setup * 🎨 Rename blogApp -> parentApp in middleware * 🎨 Co-locate all blog-related code in /blog - Move all of the blogApp code from middleware/index.js to blog/app.js - Move routes/frontend.js to blog/routes.js - Remove the routes/index.js and routes folder, this is empty now! - @todo is blog the best name for this? 🤔 - @todo sort out the big hunk of asset-related mess - @todo also separate out the concept of theme from blog * 🎉 Replace middleware index with server/app.js - The final piece of the puzzle! 🎉 🎈 🎂 - We no longer have our horrendous middleware/index.js - Instead, we have a set of app.js files, which all use a familiar pattern * 💄 Error handling fixups

@todo

closes TryGhost#4172, closes TryGhost#6948, refs TryGhost#7491, refs TryGhost#7488, refs TryGhost#7542, refs TryGhost#7484 * 🎨 Co-locate all admin-related code in /admin - move all the admin related code from controllers, routes and helpers into a single location - add error handling middleware explicitly to adminApp - re-order blogApp middleware to ensure the shared middleware is mounted after the adminApp - TODO: rethink the structure of /admin, this should probably be an internal app * 💄 Group global middleware together - There are only a few pieces of middleware which are "global" - These are needed for the admin, blog and api - Everything else is only needed in one or two places * ✨ Introduce a separate blogApp - create a brand-new blogApp - mount all blog/theme only middleware etc onto blogApp - mount error handling on blogApp only * 🎨 Separate error handling for HTML & API JSON - split JSON and HTML error handling into separate functions - re-introduce a way to not output the stack for certain errors - add more tests around errors & an assertion framework for checking JSON Errors - TODO: better 404 handling for static assets Rationale: The API is very different to the blog/admin panel: - It is intended to only ever serve JSON, never HTML responses - It is intended to always serve JSON Meanwhile the blog and admin panel have no need for JSON errors, when an error happens on those pages, we should serve HTML pages which are nicely formatted with the error & using the correct template * 🐛 Fix checkSSL to work for subapps - in order to make this work on a sub app we need to use the pattern `req.originalUrl || req.url` * 🔥 Get rid of decide-is-admin (part 1/2) - delete decide-is-admin & tests - add two small functions to apiApp and adminApp to set res.isAdmin - mount checkSSL on all the apps - TODO: deduplicate the calls to checkSSL by making blogApp a subApp :D - PART 2/2: finish cleaning this up by removing it from where it's not needed and giving it a more specific name Rationale: Now that we have both an adminApp and an apiApp, we can temporarily replace this weird path-matching middleware with middleware that sets res.isAdmin for api & admin * 🎨 Wire up prettyURLs on all Apps - prettyURLs is needed for all requests - it cannot be global because it has to live after asset middleware, and before routing - this does not result in duplicate redirects, but does result in duplicate checks - TODO: resolve extra middleware in stack by making blogApp a sub app * ⏱ Add debug to API setup * 🎨 Rename blogApp -> parentApp in middleware * 🎨 Co-locate all blog-related code in /blog - Move all of the blogApp code from middleware/index.js to blog/app.js - Move routes/frontend.js to blog/routes.js - Remove the routes/index.js and routes folder, this is empty now! - @todo is blog the best name for this? 🤔 - @todo sort out the big hunk of asset-related mess - @todo also separate out the concept of theme from blog * 🎉 Replace middleware index with server/app.js - The final piece of the puzzle! 🎉 🎈 🎂 - We no longer have our horrendous middleware/index.js - Instead, we have a set of app.js files, which all use a familiar pattern * 💄 Error handling fixups

kirrg001 reviewed Oct 4, 2016

View reviewed changes

core/client

@@ -1 +1 @@

Subproject commit 0a163d733315dfee2f5a20ea6aed41ffc7bc3bda

Subproject commit 0c2c81def806df1fa5cb0a55afff68e60883f20d

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

ErisDS force-pushed the admin branch from da0f62f to 70467db Compare October 4, 2016 17:53

ErisDS mentioned this pull request Oct 6, 2016

Change client assets to be served from /ghost/assets #7503

Closed

ErisDS force-pushed the admin branch 5 times, most recently from a36312c to 970afdb Compare October 8, 2016 16:29

ErisDS changed the title ~~[WIP] Split "admin" out into it's own app-like thing~~ Split "admin" out into it's own app-like thing Oct 8, 2016

ErisDS force-pushed the admin branch 2 times, most recently from 7db1b4c to 8833235 Compare October 8, 2016 16:39

ErisDS force-pushed the admin branch 3 times, most recently from 79ffcc0 to a6a1d1e Compare October 9, 2016 14:55

ErisDS added 4 commits October 9, 2016 15:56

💄 Separate out admin tasks

3e891f9

- cleanup to help us see what needs to be done to decouple the client & server

🎨 No more decide-is-admin

b2eb866

- replace `decide-is-admin` middleware with simple `app.set()` call - update all the places which depende on this

💄 Move admin-only middleware to admin

7dd3d07

- redirectToSetup is only needed in the admin app - this is one less confusing thing inside of core/server/middleware 🎉

ErisDS force-pushed the admin branch from a6a1d1e to 7dd3d07 Compare October 9, 2016 14:56

ErisDS mentioned this pull request Oct 9, 2016

🎨 Add support for short one-line log format #7523

Merged

ErisDS changed the title ~~Split "admin" out into it's own app-like thing~~ [WIP] Split "admin" out into it's own app-like thing Oct 9, 2016

This was referenced Oct 10, 2016

Misc Middleware cleanup #7526

Merged

🎉 Middleware refactor: Give the API its own express App #7537

Merged

🎉 🎨 ✨ Remove middleware/index.js #7548

Merged

ErisDS closed this Mar 1, 2017

ErisDS deleted the admin branch March 1, 2017 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Split "admin" out into it's own app-like thing #7484

[WIP] Split "admin" out into it's own app-like thing #7484

ErisDS commented Oct 4, 2016 •

edited

Loading

This comment was marked as abuse.

This comment was marked as abuse.

ErisDS commented Oct 4, 2016

ErisDS commented Oct 8, 2016 •

edited

Loading

		@@ -1 +1 @@
		Subproject commit 0a163d733315dfee2f5a20ea6aed41ffc7bc3bda
		Subproject commit 0c2c81def806df1fa5cb0a55afff68e60883f20d

[WIP] Split "admin" out into it's own app-like thing #7484

[WIP] Split "admin" out into it's own app-like thing #7484

Conversation

ErisDS commented Oct 4, 2016 • edited Loading

This comment was marked as abuse.

This comment was marked as abuse.

ErisDS commented Oct 4, 2016

ErisDS commented Oct 8, 2016 • edited Loading

ErisDS commented Oct 4, 2016 •

edited

Loading

ErisDS commented Oct 8, 2016 •

edited

Loading