Page has too many responsibilities which makes it difficult to reason about. This task is to break Page into the following components, update references, and add some very nice new tests:
- Page / Page.test.js: a plain model
- PageView / PageView.test.js: subclasses View
- PageJSONParser / PageJSONParser.test.js: encompasses at least the current Page.newFromJSON which parses JSON and returns a model
- PageHTMLParser / PageHTMLParser.test.js: this may have some overlap with PageView. For example, the current Page.getThumbnails parses the DOM to construct a model. Ideally, this code will accept HTML and return a Page model.
PageJSONParser looks like the easiest to tackle and may be a good place to start. PageView should probably come next so that it's easy to test the Page model without a DOM. Due to overlap with PageView, PageHTMLParser should probably be done last and live with PageView until separated.
Acceptance criteria:
- Page is a fairly plain model. There will no decrease in the amount of existing code so any model methods that existed previously will still be there.
- PageView is just another View subclass. It accepts a Page model in its constructor. No new patterns here. Just an encapsulation of view-only responsibilities.
- PageJSONParser is a simple parser. It clarifies in the comments what gateways or response formats it works with given the flexibility of the MW API. It returns a Page model.
- Some tests for each that look nice and set a good precedent that future tests can be patterned off of.
Bonus AC:
- PageHTMLParser is distinct from PageView and clarifies in the comments what HTML it can digest. For example, PageHTMLParser shouldn't need to digest a PageView since it's constructed from a Page model. However, it might need to digest a raw server rendered article to generate a Page model.
= Scope / acceptance criteria
The Page object is used in various places, so breaking changes to its API are going to be tricky and risky. I thus propose we make any changes here backwards compatibility. For example:
```
// if the new api is
new PageView( { page } )
// but the old id was:
new PageView( { title } )
// then we should
function PageView( options ) {
if ( !options.page ) {
options.page = new Page( { title: options.title } )
// message should be more friendly ;-)
mw.log.warn( "hey you! Pass the PageView a page rather than a title. Don't be lazy!" );
}
}
```
Refactors often have a tendency to grow, so we may consider this done when the following is true:
[] Page.newFromJSON is broken out into it own file
[] There are no changes to the Page contract - implicit parsing can still happen, but feel free to use mw.log.deprecate to complain to the developer that they are using code that will in future not be used.
[] Code coverage of Page.js (and any broken out classes is at 100%)
= Sign off notes
[] Page code coverage should increase (https://www.mediawiki.org/wiki/Reading/Web/Projects/Invest_in_the_MobileFrontend_%26_MinervaNeue_frontend_architecture/Progress?useskin=vector#Speed_up_unit_test_execution_and_increase_code_coverage)