BIG FAT ghost.js & index.js refactor #360
Comments
|
In my opinion, anything with a session-specificity, or an object lifecycle within the space of one request should be middleware. But if the goal isn't serving a request, or performing a function tied to a given user session, the function in question should not be middleware. We need to take advantage of the fact that we're a single long-running process, and remove as much as possible from the request chain. This'll let us do caching better, optimistic data-fetching, cross-request resource pooling, etc. And it'll lighten the load when a request comes in, allowing us to serve it faster. It strikes me that there's a some stuff which only really needs to be done once, but is happening every request, like: app.set('view engine', 'hbs');
// return the correct mime type for woff files
express['static'].mime.define({'application/font-woff': ['woff']});While there are definitely things we need to do on a request basis because (for instance) we've got specific logic for admin & non-admin components, I feel that the more we can move to one-time configuration functions, or code decoupled from requests, the better. |
fixes TryGhost#392 - adds appRoot, and uses this to calculate other paths - removes path calculations from loader - remove the themedir setting in config.. completely unnecessary - highlights just how important TryGhost#360 is
|
@tgriesser has made an interesting start: https://github.com/tgriesser/Ghost/compare/master...refactor |
|
It looks awesome @tgriesser! As an aside, one thing I'm keen to look into is whether we can somehow turn initTheme into a one time configuration stage rather than middleware. |
|
Indeed, it probably can be done, provided there is a way to change the theme without restarting the server :) It seems to me, that moving |
|
I've created a branch called 'big-fat-refactor'. Anyone and everyone is welcome to submit PRs to it with their suggestions for refactors. I've also started a wiki page with an audit of how the Ghost object is used: https://github.com/TryGhost/Ghost/wiki/Ghost.js-audit The final outcome of this refactor should result in
|
|
Audit is finished, I think it highlights lots of super-easy ways to improve the code. https://github.com/TryGhost/Ghost/wiki/Ghost.js-audit |
|
After working with the settigns api I think that all data should be retrieved from the api. There are currently different concepts of getting data:
In my opinion the api could be added to the ghost instance and then all access to data is done with ghost.api.
This would hopefully remove circluar reference of ghost.js and api.js (see mail.js) and reduce the size of ghost.js. settingsCache, dataProvider and config could be maintained within api.js and are only available with api functions. All functions that should be available externally are mapped to an HTTP end point. All other api functions should remain for internal use only. |
|
Firstly, it's absolutely correct that all of those ways of accessing data are awful, inconsistent and introducing weird and circular dependencies. Exposing it all through an API sounds like an excellent approach to solving this. The only complexity would be with initialisation. The api would require the database & models all to be setup, and they require config and settings etc. So migrations/index might have to have some special access to ensure everything gets setup in the right order. Shall I raise a new issue to do this and assign to you? |
|
Following on from where we are now in terms of this stuff, I figured I'd list out the deliverables, and turn this into an epic A lot of stuff has been done to improve this already, including getting rid of the old
|
ghost
assigned
|
@hswolff have created and assigned the death of singletons to you. Did you also want an issue around moving the register*** functions somewhere sensible? Perhaps you want to open that and I'll assign it? Want to keep the PRs small to reduce rebasing. |
|
I think this is pretty much done now 👍 Moving to 0.5 so I remember to give it a bit of a review. |
no-issue We were incorrectly mixing transactional and non-transactional operations. An e2e test in Ghost will be merged shortly which caught this problem.
Please note: This is in dire need of doing, but it is also incredibly sensitive. Whoever takes this on needs to take care not to introduce a bunch of regressions, this probably means writing quite a few additional unit tests before tackling it.
There is some discussion of this in #52
My original intention was that ghost.js would provide the API used by themes and plugins to interact with Ghost - registering helpers and filters and such like. However, it quickly spiraled into a dumping ground (my fault) and now it's the place where several things live. It's also a bloody ugly singleton.
The main things that ghost.js now seems to be made up of are:
These things should be much more clearly separated, perhaps in separate files/modules.
The app initialisation needs to be more organised. Perhaps some of it should be middleware? The rest of it should be come much more managed, such that we have a single init function on which we wait in index.js if we need to. Additionally there is the ghost.loaded promise in index.js that I don't think we need at all.
The API for themes/plugins should be disentangled such that it can be offered up to themes/plugins without them getting all the other crud / access to things they really shouldn't have.
The internal API / state should be cleanly managed. Immutable properties should be frozen or managed with closures and providing only a getter.
The singleton pattern should be ditched.
Discussion, thoughts and ideas are very much welcome on this one!
The text was updated successfully, but these errors were encountered: