apify/actor-templates

Augment templates configs with more opinionated stricter rules for actor development

Opened this issue · 3 comments

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

and also:

  • Add rule for Apify.getValue('INPUT') -> Apify.getInput()
  • async event listeners on page.on() or any other event listener that is synchronous. if it's there, ask to add try/catch block
  • for type: "module", require adding the extension for .js files (not .mjs)
  • add jsconfig.json by default, a lot of VSCode devs have no idea this exists (and they code contains 0 warnings/errors/hints) and helps a lot with "closer to tsconfig" intellisense and even a companion to eslint

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

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.

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.

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.