search

dopecodez / Wikipedia

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

Fix undefined behaviour for pages with numbers as title (e.g 7) #52

Open phiph-s opened 1 year ago

phiph-s commented 1 year ago

When requesting the page "23", it is expected to receive this page: https://en.wikipedia.org/wiki/23 Expected behavior: wiki.page("23") -> https://en.wikipedia.org/wiki/23 wiki.page(23) -> https://en.wikipedia.org/wiki/Assistive_technology

Actual behavior: wiki.page("23") -> https://en.wikipedia.org/wiki/Assistive_technology (which has ID 23) wiki.page(23) -> https://en.wikipedia.org/wiki/Assistive_technology

So there is no way to get the page for the number 23 or for example years like 1939, sine they will be treated as IDs.

It is checked whether an ID or a Title is given by isNaN(title) in utils.ts, the String "7" will be handled like a page ID. Instead check the actual type of what the function receives: return typeof title === 'string' || title instanceof String;

New behavior: wiki.page("23") -> https://en.wikipedia.org/wiki/23 wiki.page(23) -> https://en.wikipedia.org/wiki/Assistive_technology

-> Supply a number, get a page by ID, supply a string, get the page by title

phiph-s commented 1 year ago

Accidentally added a version change to the pull request... My bad

phiph-s commented 1 year ago

I saw in the Tests that you actually test against strings as IDs. If the behavior is wanted like this, I would at least consider adding parameters to the query that define whether the string should be treated as ID or title.

dopecodez commented 1 year ago

Its a very interesting point @phiph-s - apologies for the late reply i was travelling last week.

You are correct in saying that currently we do consider numbers as ids particularly. Your MR makes sense to me but i think we need some more test cases. I will work on the same in the coming weekend if you are busy with something else.

Thanks for the MR @phiph-s

phiph-s commented 1 year ago

I'm currently quite limited in time, but maybe I'm able to write some test cases. Another option would be to implement the 2 functions "pageById" and "pageByTitle", in order to not change the behavior of the current implementation of "page", as this change might break some projects that use this package.