search

dopecodez / Wikipedia

Wikipedia for node and the browser
MIT License
82 stars 19 forks source link

Implement method for mobile-html #17

Closed friendofdog closed 3 years ago

friendofdog commented 3 years ago

Takes a page title and returns mobile-optimised HTML.

closes #14

friendofdog commented 3 years ago

I made an attempt at this issue but ran into some issues with makeRestRequest(). This function calls .json() on all responses, which will not work if the response is a string (like an HTML document). I changed makeRestRequest() in ba806cc so it can handle strings (all tests still pass) but I'm not sure how this might impact other parts of the app.

dopecodez commented 3 years ago

I checked out the other functions of your forks quickly and it looks like everything is working. I'll check out the resultTypes and options and merge this tomorrow.

Feel free to change this from a draft when you are good with the changes.

friendofdog commented 3 years ago

Thanks for the feedback. I just added mobileHtml() to docs/wiki.md and un-drafted the PR.

dopecodez commented 3 years ago

I renamed the test file to match jest naming conventions and also added an extra test. I think the changes are looking good. We'll merge them once you have a chance to look through my changes.

I also added mobileHtml to the page and added a few tests.

friendofdog commented 3 years ago

I renamed the test file to match jest naming conventions and also added an extra test. I think the changes are looking good. We'll merge them once you have a chance to look through my changes.

I also added mobileHtml to the page and added a few tests.

There's actually no error thrown if the page isn't found. Wikipedia returns the below object, so I just passed it to the response.

Should I instead do something like throw new htmlError("Page not found"); if the Wikipedia API returns the "not found" object?

{
  type: 'https://mediawiki.org/wiki/HyperSwitch/errors/not_found',
  title: 'Not found.',
  method: 'get',
  detail: 'Page or revision not found.',
  uri: '/en.wikipedia.org/v1/page/mobile-html/Qqqqqqqq'
}
dopecodez commented 3 years ago

No @friendofdog , i think its fine for now every REST method is implemented that way.

We'll let it handle other errors and the Not Found can be returned like it is handled currently.

friendofdog commented 3 years ago

@dopecodez understood. This PR is okay to merge now?

dopecodez commented 3 years ago

Thanks for the PR @friendofdog , your work keeps this repo moving.

We should definitely do something about the coverage failing like discussed in https://github.community/t/make-secrets-available-to-builds-of-forks/16166. I'll open a separate issue for this as this makes PRs look bad.