Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
pocesar opened this issue Apr 12, 2022 · 3 comments
Labels
t-tooling Issues with this label are in the ownership of the tooling team.

Comments

@pocesar
Copy link

pocesar commented Apr 12, 2022

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

@metalwarrior665
Copy link
Member

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
Copy link
Member

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
Copy link
Author

pocesar commented Apr 13, 2022

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.

@B4nan B4nan added the t-tooling Issues with this label are in the ownership of the tooling team. label Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-tooling Issues with this label are in the ownership of the tooling team.
Projects
None yet
Development

No branches or pull requests

3 participants