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

Theme API Audit #7491

Closed
10 of 14 tasks
ErisDS opened this issue Oct 5, 2016 · 6 comments
Closed
10 of 14 tasks

Theme API Audit #7491

ErisDS opened this issue Oct 5, 2016 · 6 comments
Assignees
@ErisDS @kirrg001

Comments

@ErisDS
Copy link
Member

ErisDS commented Oct 5, 2016 •
edited by kirrg001
Loading

I'm currently working through, reviewing all the open (and many closed) issues & PRs with the themes label. In doing this, I'm collecting together a set of quick wins & bigger changes I want to make as part of the alpha process.

This should feed into a set of subtasks/issues, and ultimately into a blog post about what's changing.

  • review open issues w/ theme label

Known things which need extended reviews:

  • asset helper + envs
  • excerpts?
  • deprecations

Other tasks:

  • write up the new package.json spec
  • create a validator for the new package.json spec in gscan
  • write a blog post about all the changes

The plan is that, with the exception of features that have been deprecated for over a year, like some of the body classes and page_url, although some changes may be required for a theme to work with Ghost 1.0.0, themes that work with 1.0.0 should also work on the 0.11 LTS branch. E.g, themes can be prepared in advance of the release of Ghost 1.0.0.

  • Remove the deprecated features
  • Get rid of validateActiveTheme
  • scan themes with gscan when copied themes manual to themes folder (we could scan the active theme on the first http request to ensure it's 100% valid or similar ideas)

Error handling:
@ErisDS ErisDS added this to the 1.0.0-alpha.4 milestone Oct 5, 2016
@ErisDS ErisDS self-assigned this Oct 5, 2016
@ErisDS ErisDS mentioned this issue Oct 5, 2016
Merged
@kirrg001 kirrg001 mentioned this issue Oct 11, 2016
Merged
@ErisDS ErisDS mentioned this issue Oct 13, 2016
Merged
13 tasks
ErisDS added a commit to ErisDS/Ghost that referenced this issue Oct 13, 2016
@ErisDS
💄 Error handling fixups
53b7b70 
refs TryGhost#7491, refs TryGhost#7488
kirrg001 pushed a commit that referenced this issue Oct 13, 2016
@ErisDS @kirrg001
🎉 🎨 ✨ Remove middleware/index.js (#7548)
4411f82 
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
@ErisDS ErisDS modified the milestones: 1.0.0-alpha.4, 1.0.0-alpha.6 Oct 14, 2016
@ErisDS ErisDS modified the milestone: 1.0.0-alpha.6 Oct 24, 2016
mixonic pushed a commit to mixonic/Ghost that referenced this issue Oct 28, 2016
@ErisDS @mixonic
🎉 🎨 ✨ Remove middleware/index.js (TryGhost#7548)
1a5b71b 
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
geekhuyang pushed a commit to geekhuyang/Ghost that referenced this issue Nov 20, 2016
@ErisDS @geekhuyang
🎉 🎨 ✨ Remove middleware/index.js (TryGhost#7548)
44c06d2 
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
Copy link
Contributor

kirrg001 commented Jan 16, 2017 •
edited by ErisDS
Loading

Some things in this issue are a little unclear to me, but let me write down tasks and questions.
This comment can contain misinterpreted facts - can be corrected/updated.

Questions:

  1. remove deprecations is marked as finished - when and how was this done?
  • pageUrl
  • remove old body classes
  • content=0 hack
  1. should we require a package.json for 1.0 themes?Yes and no. There should be a hard and soft configuration in gscan. Hard for marketplace, soft for uploading themes into Ghost.

Required Tasks (not backward compatible)

Optional Tasks:

@kirrg001 kirrg001 self-assigned this Jan 18, 2017
@ErisDS ErisDS mentioned this issue Feb 10, 2017
Closed
@ErisDS
Copy link
Member Author

ErisDS commented Feb 27, 2017 •
edited
Loading

The work for this issue expanded a bit, because all of the code that currently makes themes work is split across config & settings in awkward ways. It was time to solve these problems so that we can move forward cleanly.

Two other new items:

@ErisDS ErisDS mentioned this issue Mar 2, 2017
Merged
ErisDS added a commit to ErisDS/Ghost that referenced this issue Mar 7, 2017
@ErisDS
🎨 Split asset helper for admin & theme
4084a86 
refs TryGhost#7488, TryGhost#7491

- At the moment, we use the same asset helper function for both admin and theme
- Both functions are (rightly or wrongly) a wrapper around meta.getAssetUrl()
- The usecases in these contexts are v. different:
    - asset for the admin is used only in the generated index.html file
    - ghost="true" is always true in this case, except for the favicon which is ALREADY a separate case
    - minifyInProduction is only used to the admin panel, not documented or supposed to be used for themes
- For themes, the helper doesn't take any arguments, it should just call to getAssetUrl()
  If we do want to introduce some sort of .min path adjuster we should give it a better name!
- For the admin panel, minifyInProduction means exactly what it says - minify if env === production,
  it makes no sense IMO to move this to config because we control it and also plus "minifyAssets" made it
  sound like we had an asset pipeline when all this did was change the output from .js to .min.js or .css to .min.css
@ErisDS ErisDS mentioned this issue Mar 7, 2017
Closed
@ErisDS
Copy link
Member Author

ErisDS commented Mar 13, 2017 •
edited
Loading

I have an open PR for everything now except for

As I forgot about these 😞 they are small at least.

Notes:

  • All the stuff to do with the package.json spec & gscan will be part of the migration path work I think?
  • I am not sure what "Revisit stack printing" was? (mentioned in original issue)
  • All the stuff to do with excerpts was not scheduled for 1.0

I would also have liked to have made more progress with the asset helper, at least we have a plan for not sharing it between themes and admin again. More improvements can come in future without breaking changes, I think.

ErisDS added a commit to ErisDS/Ghost that referenced this issue Mar 14, 2017
@ErisDS
🔥 Remove legacy content zero hack
9f92d8f 
refs TryGhost#7491

- this hack is so legacy I almost forgot about it 😈
- in the beginning of Ghost there were no post images
- someone figured out you could do {{content words="0"}} and it would pull out the first image in your post
- this was never documented, but enough theme developers found it that when we upgraded downsize to get rid of the bug
- we needed to add a hack to keep compatibility.
- This has to die in 🔥  for Ghost 1.0
@ErisDS ErisDS mentioned this issue Mar 14, 2017
Merged
kirrg001 pushed a commit that referenced this issue Mar 14, 2017
@ErisDS @kirrg001
🔥 Remove legacy content zero hack (#8156)
5605c96 
refs #7491

- this hack is so legacy I almost forgot about it 😈
- in the beginning of Ghost there were no post images
- someone figured out you could do {{content words="0"}} and it would pull out the first image in your post
- this was never documented, but enough theme developers found it that when we upgraded downsize to get rid of the bug
- we needed to add a hack to keep compatibility.
- This has to die in 🔥  for Ghost 1.0
@ErisDS
Copy link
Member Author

ErisDS commented Mar 17, 2017

Further update - this is mostly as done as it will be for 1.0. Some cleanup work to do around the asset helper now that the admin-server split is done and lots of documentation to do along with updates to gscan.

@kirrg001
Copy link
Contributor

@ErisDS Could we raise one or two separate issues with the missing pieces and close this big issue?
Let me know 👍

ErisDS added a commit to ErisDS/Ghost that referenced this issue Mar 21, 2017
@ErisDS
🎨 🚨 Split theme tests, clean config & add tests
ee728f2 
refs TryGhost#7491

- split themes_spec up into several files
- clean up the code for configuration
- ensure its tested
@ErisDS ErisDS mentioned this issue Mar 21, 2017
Merged
kirrg001 pushed a commit that referenced this issue Mar 22, 2017
@ErisDS @kirrg001
🎨 🚨 Split theme tests, clean config & add tests (#8205)
317daf5 
refs #7491

- split themes_spec up into several files
- clean up the code for configuration
- ensure its tested
This was referenced Mar 23, 2017
Closed
Closed
Closed
@ErisDS
Copy link
Member Author

ErisDS commented Mar 23, 2017

Have raised #8219, #8221 & #8222 to capture what's left here.

@ErisDS ErisDS closed this as completed Mar 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ErisDS ErisDS

@kirrg001 kirrg001

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@ErisDS @kirrg001