- When we handle errors in Ghost, we are supposed to use a pattern of supplying 3 messages:
- message: what went wrong
- context: details about why how or where the error happened
- help: where the user can go to get help with this error
- We do this in many places and our JSON error handler and CLI error logging tools are designed to output this extra information
- However, stack traces, which start with message as the first line and then output the stack are totally missing this
- By injecting the additional messages into the stack once an error has been "ghostified" we should get clearer messages everywhere
Notes:
- I've additionally injected a "Stack Trace:" line that makes it easier to read the error vs the stack
- This code looks a little weird because the lines are inserted backwards, but that allows us to always to the insert at position 1 as per the comment,
so we don't have to keep track of whether we already injected something or not
refs: 2af9e2e12
- This new HTMLErrorRenderer is borrowed heavily from finalHandler
- This is the module that express uses to render errors if there is no custom errorhandler
- It just renders a really simple html page wrapping err.stack in a <pre>
- This results in a nicely formatted, but unstyled error page
- I also updated BasicErrorRenderer to use the same res.statusCode + err.stack pattern rather than err.message
Note: This error renderer is _only_ used for renderering errors on the `/ghost/` route
- In almost all cases, errors here are rendered by Ember
- The only error that can be rendered here is a missing template error see: 2af9e2e12
- If the admin templates default.html or default-prod.html are missing, don't throw a 500
- Instead throw a well considered 400 error with extra help for what to do to fix it
- Reduced our maintenance middleware code down to the bare minimum!
- We have an old maintenance middleware in place to handle when a site is forcibly put into maintenance mode, or the urlService hasn't finished booting
- This maintenance middleware was mounted on every sub app, instead of globally for reasons I no longer remember
- Recently, we introduced a new, static version of maintenence middleware to show during the boot process so we can get the server started earlier & not drop requests
- This version has its own HTML template and doesn't depend on any of Ghost's error rendering code
- To simplify and help with decoupling, this commit merges the two middleware, so that the new independent & static middleware renders its template for any one of the 3 possible maintenance modes
- It only needs to exist in the top level app 🙌
TODO: move the maintenance middleware to its own file/package so it's not part of the app.js as that is weird
- throughout the theme activation flow there are several missing awaits and necessary async keywords
- we should be waiting on these processes, not letting them complete indeterministically
refs https://linear.app/tryghost/issue/CORE-35/refactor-route-and-redirect-settings
- It's a step to making the module follow class+DI pattern before fully extracting it into an external libarary
- Reminder, doing in Ghost repo instead of substituting big chunks all at once to have clear history of how the service evolved prior to the extraction into external lib!
no issue
When switching the oembed service to async/await the error handling was not correctly refactored. `this.errorHandler(url)` was returning a curried function so it could be used as `.catch(this.errorHandler(url))` but that's not how it's being used after the async/await change meaning we were returning a function rather than the result of that function.
- `this.errorHandler(url)` is now only used in one place where `url` is available so removed the method and moved the body of the curried function inline into the `catch` handler
- added a message to the logged error so it's more clear what the log refers to
refs https://github.com/TryGhost/Toolbox/issues/135
- Without sensible defaults the web app was not initializing either the backend nor the frontned parts of the application. Fixed the defaults so the problem doesn't happen again and optimized mock-express-style initialization to only initialize the frontend routing
no issue
- `startsWith` method is way easier to read and understand. also, **probably** has better performance comparing to building up a regexp and then matching
refs https://github.com/TryGhost/Toolbox/issues/135
- When instantiating new Urls/Resources object in the UrlService's init on every test suite start it was loosing track of past events (softReset wasn't doing the cleanup properly). Not initializing new classes "fixes" the problem partially, by not loosing track of those event listeners. The real fix should be proper even listener cleanup on every soft reset!
refs https://github.com/TryGhost/Toolbox/issues/135
- Allows to turn off overwriting urls/resources JSON file caches on testing environment. This is needed to have predictable state when running multiple test suites that stop the Ghost process and try to persiste URL cache.
refs https://github.com/TryGhost/Toolbox/issues/135
- This extracts the file storage knowledge out of the URL Service an allows to have optional features based on the environment - for example turning off writing cache for when running tests
refs https://github.com/TryGhost/Toolbox/issues/135
- To be able to reliably start ghost instance without a frontend the process needs access to urls/resources caches
- Storing the configuration in "paths" for now as there's no better place for it untill we are able to mock the content folder in pre-boot
refs https://github.com/TryGhost/Toolbox/issues/135
- These flags are meant to control initialization of sections of the boot sequence depending on the needs - with or without bakend (API)/frontend (public handlebars site)
- Ideally these flags should not be passed deep into the components, and if the are (like in the web/parent/app case) it's a smell that we need to move things up into the boot process!
- These two things are meant to improve performance at the cost of reliability.
- Perfect for testing, however I think they make a minimal impact on modern SSDs :(
- Still worth a shot to see if it helps with CI
refs https://github.com/TryGhost/Toolbox/issues/136
- `cheerio` isn't needed during the boot but it takes time and memory to
load the library
- this commit moves `cheerio` requires later into the code to when they
are needed
no issue
- if we encounter an unexpected error whilst fetching embed details we return a generic validation error so we're not leaking any details about the URL that is being hit, however that meant the error logs were only showing validation errors making debugging difficult
- added explicit logging of the unexpected error before throwing the generic validation error
- These are simple functions that get data from config in a specific format
- They are also used by the topmost part of the application
- Config helpers seems like a reasonable fit to get them out of the web folder
- Functions have also been renamed to try to get them to make more sense
refs https://github.com/TryGhost/Team/issues/1211
Added ?format=json to the URL in an attempt to mitigate any issues with
weird caching and receiving HTML rather than JSON.
Used `type` in place of `card_type` to closer follow how the bookmark
card/embed works.
Removed the html & width/heihgt properties which are not needed at all.
refs https://github.com/TryGhost/Team/issues/1217
- moved top-level `tenorApiKey` to `tenor:apiKey` and added `tenor:contentFilter`
- added base config to `defaults.json`
- updated `public-config.js` and API output serializer to use the new top-level `tenor` key
https://github.com/TryGhost/Toolbox/issues/130
The transaction no longer commits in the promise chain, which wasn't
valid logic for a transaction, since it is commited automatically when
the promise chain resolves, and rollsback automatically when the
promise chain rejects.
This makes code which fails during the transaction error in the right
place, instead of getting stuck here. (Especially good for writing
tests).
The tests for this code can now live in the integration folder.
- This is a minor bugbare, but it will affect some configuration I'm about to do for c8
- I've been wanting to do it for ages, middleware is plural all on it's own so it's an odd affectation in our codebase
- This also only exists in 2 places, everywhere else we use "middleware"
- Sadly it did result in a lot of churn as I did a full find and replace, but consistency is king!
- this keeps production and test fixtures separate, so that changing the prod fixtures doesn't change the shape of our tests.
- we may still want to test that the production fixtures do what we expect, but that can be handled in a separate integration test, by specifically setting the fixture path
refs: https://github.com/TryGhost/Toolbox/issues/133
- instead of just a collection of utils, we now have a class that manages fixtures
- this should allow us to change the path to fixtures, e.g. between prod/dev and test, so that different fixtures can be loaded by default
- also makes it easier to test the fixture manager code itself
refs https://github.com/TryGhost/Toolbox/issues/127
- New instances or Urls and Resources during init were messing up test suites without an obvious solution. Moved them to only be triggered when the cache is present.
- Note the e2e-frontend tests are failing if thie initialization happens every time when init is called. There seems to be some reliance on Resources being the same intsance between "softReset" which seems wrong, but wasn't able to track it down definitevely
refs https://github.com/TryGhost/Toolbox/issues/127
- The resource cache is needed to have quick and reproducible state of the resouces tied to the urls instead of waiting for the db queries to finish.
- Allows to use UrlService without any database connection at all - useful for unit testing
- At the moment there's no way to see in the logs when the URL Service finally finishes
- This is the moment when Ghost stops serving 503s
- Adding this log line so it's clear to see, which migh be useful whilst were refactorising
refs https://github.com/TryGhost/Toolbox/issues/127
- The "withUrlOptions" is used internally in the constructor by the frontend's routers. To avoid passing around a whole function from the frontend routers duplicated the functionality from there.
refs 5a62253466
- The UrlService was relying on a "hidden" identifier field that was passed along with a router object. Now it's passed as an explicit parameter from the "frontend" to the backend's UrlService
refs https://github.com/TryGhost/Toolbox/issues/127
- Passing around whole instance of a frontend router was an overkill when there are only 3 static pieces of information that needed to be loaded. Extracting the router out makes the UrlGenerator way more readable, tests slimer, and the memory footpring of the process should be slightly lighter
- The toString overloading didn't make sense at the time of this refactor, maybe if there's a concrete usecase we could resurect it in a form of passing in a router's name or something.
refs https://github.com/TryGhost/Toolbox/issues/127
- This is an effor t to define a precise set of data needed for the UrlGenerator to function, which should help with decoupling it from the frontend routes
- This is almost the last piece to free us up from the massive "router" object that has been passed around
refs https://github.com/TryGhost/Toolbox/issues/127
- This is an effor t to define a precise set of data needed for the UrlGenerator to function, which should help with decoupling it from the frontend routes
refs https://github.com/TryGhost/Toolbox/issues/127
- This is an effor t to define a precise set of data needed for the UrlGenerator to function, which should help with decoupling it from the frontend routes
refs https://github.com/TryGhost/Team/issues/1211
Instead of rendering the HTML as an embed - we will send back the
necessary data. This will allow us to keep all the knowledge of HTML
structure in the Koenig repository.
