Scraper for basic information - #1
Conversation
|
I am sorry for notifying you so early, forgot there is a button to create the PR as Draft to begin with. |
3cb2181 to
62367e8
Compare
metalwarrior665
left a comment
There was a problem hiding this comment.
Looks great, just a few small remarks.
| "type": "boolean" | ||
| } | ||
| }, | ||
| "required": ["category", "title", "url", "author", "createdAt", "lastPostAt", "posts", "isAnnouncement"] |
There was a problem hiding this comment.
Having required fields in the dataset output is considered quite dangerous. If any of them is missing, the pushData will throw and you will loose the data. And websites change so one field might be broken. Generally, our approach is to rather push incomplete data and catch the issue in periodic tests/monitoring.
Using required fields is possible but we will only do it when it is validated with TypeScript (library to ensure 1:1 between TS and schema was recently released) and we can ensure these fields are always possible. E.g. currently it is usually not the case because we are pushing "error items" into the default dataset. We want to move to secondary dataset (also a new feature) soon
There was a problem hiding this comment.
I forgot about partially pushing data, and it makes sense. Removed the required field from the schema and added partial push of data on some routes. There is no case where partial data push makes sense to me, so that feature is not used.
| "useApifyProxy": true, | ||
| "apifyProxyGroups": [] | ||
| } | ||
| }, |
There was a problem hiding this comment.
In non-generic (that don't work on any website) public Actors, we don't expose the proxy to the user. Usually, we set either datacenter or residential depending which works. Users have no idea what proxies they should use.
There was a problem hiding this comment.
Thank you, change it o that the proxies are not in the input
| const $views = $topic.find(selectors.topic.views).first(); | ||
| const views = parseNumber($views.text().trim().split(' ')[0]) ?? 0; | ||
|
|
||
| const $lastPost = $topic.find(selectors.topic.lastPost).first(); | ||
| const lastPostAt = $lastPost.find('time').attr('datetime') || $lastPost.text().trim(); | ||
|
|
||
| const $author = $details.find(selectors.topic.author).first(); | ||
| const author = $author.text().trim() || 'Unknown'; | ||
|
|
||
| const $createdAt = $details.find(selectors.topic.createdAt).first(); | ||
| const createdAt = $createdAt.attr('datetime') || $createdAt.text().trim(); |
There was a problem hiding this comment.
Lot of these variables are not reused so unless the extraction is too long, I would try to inline them to the result object so that the reader doesn't need to carry the variables in their head checking if they will be used somewhere.
There was a problem hiding this comment.
Where it made sense I moved it, but not everything because it made it less readable sometimes when there were a lot of function calls on one line because of the formatter.
| urls: [url], | ||
| label: Label.TOPIC_DETAIL, | ||
| userData, | ||
| }); |
There was a problem hiding this comment.
First buffer all the URLs into an array and then enqueue them at once to only do 1 API call instead of many. The same applies for pushing data
There was a problem hiding this comment.
Had to buffer all the Request objects instead of URLs because of each URL having different user data, but the push is now in bulk.
| import type { UserData } from '../types/index.js'; | ||
| import { Label } from '../types/index.js'; | ||
|
|
||
| export const forumSearchStartHandler = async (ctx: CheerioCrawlingContext) => { |
There was a problem hiding this comment.
You are missing the userData type here
There was a problem hiding this comment.
Added the missing typing
| await enqueueLinks({ | ||
| urls: [nextPageUrl], | ||
| label: Label.TOPIC_DETAIL, | ||
| userData: topic, |
There was a problem hiding this comment.
Instead of aggregating the data and pushing at last page it might be better to push each small thing separately? Usually we try to push the smallest thing (e.g. one dataset item = one review) so we don't have to aggregate anything and it has simple schema. But in some more complex use-cases aggregation makes sense. Just be aware that single dataset item can be max 9MB.
There was a problem hiding this comment.
I was thinking of one item being the topic itself. But here there might be issue with the size limitation which I forgot about. Might check it out if there are bigger file regularly and change it around.
| const normalized = value.replace(/[,-]/g, ''); | ||
| const parsed = Number.parseInt(normalized, 10); | ||
| return Number.isNaN(parsed) ? null : parsed; | ||
| }; |
There was a problem hiding this comment.
I think your utils are small enough for them to be a single file. Many files can be harder to navigate.
Generally, I recommend starting with fewer files/folders and then refactoring out if the project gets bigger. Some projects never grow in features so there is no need to prepare "future-proof structure".
There was a problem hiding this comment.
I found them distinct enough to have them in separate files, but I understand the problem this might present. Will merge them into one utils file.
| proxyConfiguration, | ||
| requestHandler: router, | ||
| // this is 2 requests a second, this should be fine and not overload the site since it is not so frequently used one | ||
| maxRequestsPerMinute: 120, |
There was a problem hiding this comment.
A bit of issue with this option is that (I think) it doesn't distribute the requests in the minute, it will just stop processing once done. So it could still lead to high bursts at the start (not sure how likely it is in practice). You could solve that by adding maxConcurrency limit as well.
There was a problem hiding this comment.
yes I noticed that it just does huge burst at the beginning and then it slows down. Will try to do this with the maxConcurrency setting.
| A topic is a page with posts provide by users. | ||
|
|
||
| Start a new [web scraping](https://apify.com/web-scraping) project quickly and easily in TypeScript (Node.js) with our empty project template. It provides a basic structure for the Actor with [Apify SDK](https://docs.apify.com/sdk/js/) and allows you to easily add your own functionality. | ||
| ## Inputs |
There was a problem hiding this comment.
We generally prefer to link the Input page to the Actor Store page, which is derived from the inpjt schema. That way there is only single source of truth for the descriptions, you will often forget to change the readme.
There was a problem hiding this comment.
I linked the input page statically in the README.
62367e8 to
3f93dfc
Compare
3f93dfc to
5b471f8
Compare
5b471f8 to
e4f523e
Compare
e4f523e to
50a0de0
Compare
metalwarrior665
left a comment
There was a problem hiding this comment.
Thanks! Please don't force-push after a review, it makes seeing new changes harder.
Oh I did not know GitHub removes the old lines from the comment (GitLab kept them from what I remember), will adjust workflow for next PR. |
Now I see you already mentioned it last week in another review. I did not notice it. Will change it. |
Because I can not assign reviewers without you being collaborators first I tag you in this PR.
@oklinov @metalwarrior665
Was instructed by Ondro to also tag @JuanGalilea