Page MenuHomePhabricator

Separate Page into components and increase its code coverage
Closed, DuplicatePublic


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.


  • T206036 has been completed and this task updated with the outcomes

Proposed separations:

  • 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.
  • 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 ( ! ) { = 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

Event Timeline

Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptOct 5 2018, 3:20 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

let me catch up first and work out how this fits in with the other work. We have lots of things relating to the project in upcoming so no rush!

Jdlrobson renamed this task from Separate Page into components to Separate Page into components and increase its code coverage.Nov 2 2018, 3:49 PM
Jdlrobson updated the task description. (Show Details)

^ @Niedzielski my big concern reading through this is breaking the existing contract (which would require touching a bunch of files in Minerva). To avoid that, I propose we provide some backwards compatibility for PageView to construct pages if not passed and warn. What do you think about taking this approach? To me that mitigates a lot of the risk/time we spend here with a productive outcome.

Jdlrobson changed the task status from Open to Stalled.Nov 6 2018, 6:35 PM

Stalling until the spike has taken place. I think right now @Niedzielski and I have the most knowledge in this area, and I'm hoping the spike will increase knowledge of the complexity of these components. Have left a note in spike to reopen this when done as part of sign off.