mwoffliner icon indicating copy to clipboard operation
mwoffliner copied to clipboard

Remove MWCapabilities

Open kelson42 opened this issue 4 years ago • 1 comments

MWCapabilities is a dedicated class instanciated within Downloader. This is not the right place. This should be available in the Mediawiki class.

I want to have these 4 method, implemented as lazy in Mediawiki:

  • hasVeApi()
  • hasCoordinatesApi()
  • hasDesktopRestApi()
  • hasMobileRestApi()

Probably the a lazy method should be as well implemented to get the MainPage:

  • getMainPage()

Remark: This is a necessary step to re-architecture the Mediawiki-Downloader-Renderer

kelson42 avatar Jan 02 '21 16:01 kelson42

This issue has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

stale[bot] avatar Mar 20 '21 00:03 stale[bot]

@uriesk @pavel-karatsiuba IMO one of girst things to do in 1.13.0 before implementing support of any new backend. Do you agree? Have other comments?

kelson42 avatar Jan 26 '23 18:01 kelson42

I agree. And currently we getArticleUrl by appending the articleId to a baseUrl (which got choosen based on the Capabilities). But not all APIs will always allow us to do that. I would like a concept similar to this:

const apis = {
  'ACTION': {
    getArticleUrl: (articleId) => {
      return `${baseUrl.href}action=visualeditor&mobileformat=html&format=json&paction=parse&page=${encodeURIComponent(articleId)}`;
    },
  },
  'RESTV1': {
    getArticleUrl: (articleId) => {
      return `${baseUrl.href}/api/rest_v1/${encodeURIComponent(articleId)}/html`;
    },
  },
}

const choosenEndpoint = apis['ACTION'];

const url = choosenEndpoint.getArticleUrl('Article_Title');

uriesk avatar Jan 26 '23 19:01 uriesk

@uriesk I think we are on the same wave length! Before building any new API endpoint support, we need first to reorganise the current modules in a way content endpoints calls an response treatment are modularised, ie. share AFAP a common interface/API for the rest of MWoffliner.

That way it will be easier to understand an test.

I want to open a dedicated ticket for that for a long time but actually I'm not sure how the fix should exactly look like... so I have open only this small one which was more obvious to me.

kelson42 avatar Jan 27 '23 06:01 kelson42

@pavel-karatsiuba Here we will need a rearchitecturing proposal from you.

kelson42 avatar Feb 17 '23 13:02 kelson42

@kelson42 Your proposed changes are a good and easy step to start changing architecture. If we start from these changes then makes sense to decompose whall Downloader class and leave here only the methods needed to download content. And other methods need to be moved to other places. Method getCompressedBody which uses imagemin should be separated. Methods to serialize and deserialize Url can be separated into another class.

pavel-karatsiuba avatar Mar 27 '23 17:03 pavel-karatsiuba

@pavel-karatsiuba OK, but what I ask is a full proposal so I can figure out how it will look like at the end.

kelson42 avatar Mar 27 '23 18:03 kelson42

@kelson42 I have made a picture with propositions to change Downloader.ts and redesign the file structure. All changes are directed to the logical grouping the similar functions by modules. In the future, each group should be covered by tests. And tests also should be grouped. https://drive.google.com/file/d/1Fncde4_gzHPol-jAWVlQALHx1XLmZ1m6/view?usp=sharing

pavel-karatsiuba avatar Mar 28 '23 13:03 pavel-karatsiuba

@pavel-karatsiuba Where are the differents modules corresponding to the differrent mediawiki endpoints mwoffliner can deal with to retrieve the articles content?

kelson42 avatar Apr 04 '23 05:04 kelson42

Right now all Wikimedia uses the same endpoint and uses the same API. Before designing the classes structure for this situation, I need to know which parts will be different. So the next part of designing needs to have a description.

pavel-karatsiuba avatar Apr 06 '23 19:04 pavel-karatsiuba

Right now all Wikimedia uses the same endpoint and uses the same API.

I don't know what a "Wikimedia" is... and actually we are using AFAIK many different API to retrieves the HTML (see function checkCapabilities(). We want to:

  • deprecate one and introduce a new one with https://github.com/openzim/mwoffliner/issues/1664
  • introduce another one https://github.com/openzim/mwoffliner/issues/1601

I think you have enough information to design a common interface for all of them, so the rest of MWoffliner can just decide which module to deal with but wihtout having to change the rest of the code deal with the module.

kelson42 avatar Apr 10 '23 11:04 kelson42

We are using mwCapabilities to define how will be downloaded and rendered article. We need to create the Article module. In this module, we need to have the function to set the apiURL, because apiURL depends on mwCapabilities. Also, this module should have the function setArticleRenderer() which should contain logic to select the article renderer function depending on mwCapabilities. Finally, the Article module should have getArticle() and renderArticle() functions which should use apiURL and then articleRenderer. Example what should be in the Article module:

let articleRenderer: ArticleRenderer; // function to render the Article
let apiUrl; // URL to Wikimedia API which depends on mwCapabilities

export const setApiUrl = () => {
  // set apiUrl depends on mwCapabilities
  // use logic similar to Downloader.checkCapabilities() + Downloader.setBaseUrls()
}

export const setArticleRenderer= () => {
  // set articleRenderer depends on mwCapabilities
}

export const getArticle= () => {
  // use apiUrl to download Article Content
  // execute renderArticle()
}

const renderArticle= () => {
  // use articleRenderer to render downloaded Article
}

Different articleRenderer functions should be placed in the ArticleRenderer module.

pavel-karatsiuba avatar Apr 11 '23 19:04 pavel-karatsiuba

@pavel-karatsiuba Your last comment goes against the task description and Mediawiki properties should not be part of an Article class (but a Mediawiki class).

kelson42 avatar Apr 12 '23 13:04 kelson42