apify / actor-templates

This project is the :house: home of Apify actor template projects to help users quickly get started.
https://apify.com/
25 stars 15 forks source link

Augment templates configs with more opinionated stricter rules for actor development #89

Open pocesar opened 2 years ago

pocesar commented 2 years ago

this: https://maximorlov.com/linting-rules-for-asynchronous-code-in-javascript

and also:

this means it shouldn't be added to top level @apify/eslint-config because there will be many rules that only make sense while dealing with the SDK on a daily basis

metalwarrior665 commented 2 years ago

We discussed many times that we should have a @apify/actor-eslint-config or something similar that will inherit from @apify/eslint-config.

I think from SDK v3, we plan to fully migrate to ES modules (which we should probably do a long while ago).

Not sure about jsconfig.json, we would definitely need to disable the all the implicit any warnings. Better go full Typescript in that case and we don't want to force all users to necessarily do that.

metalwarrior665 commented 2 years ago

I read the article and I think there isn't much useful rules for writing actors, very rarely you would need to construct promises by hand via new Promise. And some rules we explicitly want to disable like no-return-await for stack traces, no-await-in-loop for a clean and predictable serial execution.

pocesar commented 2 years ago

dealing with events (and listeners), dealing with callbacks, this is very common to have new Promise(async (resolve, reject) => {}); and try to async/await there there it should be an async IIFE:

let c = 0;
return Promise.race([
    new Promise(async (resolve) => {
        while (c < 10) {
          await page.doSomeScrolling();
          c++; 
          await el = page.$('#id');
           if (el) {
               resolve(el);
               break;
           }
         }
     }),
     sleep(30000),
]);

can be solved by IIFE (that it's a promise without the callbacks)

let c = 0;
return Promise.race([
    (async () => {
        while (c < 10) {
          await page.doSomeScrolling();
          c++; 
          await el = page.$('#id');
           if (el) {
               resolve(el);
               break;
           }
         }
     })(),
     sleep(30000),
]);

for page events:

const f = (page) => {
  return new Promise((resolve, reject) => {
      page.on('popup', (newPage) => {
         newPage.waitForNavigation().then(() => resolve(newPage), reject);
      });
  })
}

also I've seen this quite a lot:

await Promises.all(requests.map((req) => requestQueue.addRequest(req)));
// or the good intent one, they are the same
requests.forEach(async (req) => {
   await requestQueue.addRequest(req);
});

I've also seen 4, 5, 7, 10 (people don't know there are fs.promises.readFile). the Typescript ones are almost impossible to make work on plain JS, so that can be left for TS only templates. and unless we change the ergonomics in SDK 3, we will keep seeing this, so we need better rules.