Re-added missing 'onProgress' method to new Scraper interface #774
Conversation
| @@ -137,6 +137,7 @@ export interface ScraperScrapingResult { | |||
|
|
|||
| export interface Scraper<TCredentials extends ScraperCredentials> { | |||
| scrape(credentials: TCredentials): Promise<ScraperScrapingResult>; | |||
| onProgress(func: (companyId: CompanyTypes, payload: {type: ScraperProgressTypes}) => void): void; | |||
There was a problem hiding this comment.
I think we need to add the following as well:
- triggerTwoFactorAuth
- getLongTermTwoFactorToken
And protect/add here also
- fetchData
- login
- terminate
There was a problem hiding this comment.
Can you also add a test similar to what I did in my closed PR?
There was a problem hiding this comment.
To be honest, I think using typescript is enough in this case - your test would have passed before and after my change, if the test was written in javascript - it provided value for you because it probably didn't compile.
The real problem is that "onProgress" is a undocumented and untested public API, and we should write a single real test for it (In a future PR 😉 )
There was a problem hiding this comment.
I agree that tests are not the best place to test types, but having a .ts test for the public API that will explicitly have the usage assumptions will make a shorter dev loop if it fails in israeli-bank-scrapers and not in dependent packages.
The real problem is that "onProgress" is a undocumented and untested public API, and we should write a single real test for it (In a future PR 😉 )
I agree
There was a problem hiding this comment.
@daniel-hauser @orzarchi can I merge the PR?
There was a problem hiding this comment.
@daniel-hauser I've added your test
@esakal I think we're good to go
|
🎉 This PR is included in version 3.9.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Fixes #772.
This PR also hides two other internal methods that were public, to prevent future confusion