search

apify / browser-pool

A Node.js library to easily manage and rotate a pool of web browsers, using any of the popular browser automation libraries like Puppeteer, Playwright, or SecretAgent.
88 stars 14 forks source link

refactor: make all the utils TS #31

Closed vladfrangu closed 3 years ago

vladfrangu commented 3 years ago

Will depend on https://github.com/apify/apify-shared-js/issues/131

CC: @pocesar @B4nan @mnmkng

pocesar commented 3 years ago

@vladfrangu there's #24 most of it is done, you can use that

vladfrangu commented 3 years ago

yeah, I'm planning to for the bigger classes since I saw you did a lot with generics, unless you meant something else! 😋

mnmkng commented 3 years ago

@B4nan could you please check this?

@vladfrangu sorry for the long delay, https://github.com/apify/apify-shared-js/issues/131 is closed now and the shared repo is fully in TS, you can continue with the work.

vladfrangu commented 3 years ago

@B4nan Should I disable the import/extensions rule globally? It's not needed at all in JS/TS, and from one of your commits you even removed the extension from some imports 👀

I can PR it to the eslint config repository if that's desired :D

B4nan commented 3 years ago

afaik in TS we should not use extensions in imports, or something changed? :] i know they are required to use ESM, but we are not at that boat just yet and i expect TS to handle that for us anyway

but i dont get the context, do we have some extensions in imports in the repo?

vladfrangu commented 3 years ago

but i dont get the context, do we have some extensions in imports in the repo?

On the JS side it's complaining that they are missing, hence why I'm asking what I should do

image

B4nan commented 3 years ago

idk, i guess we want them in the JS files, but there should be no JS (source) files once we are done with the conversion, right? :]

i'd rather not adjust the linter rules for JS files without knowing more context

vladfrangu commented 3 years ago

Alrighty! Then this PR is review-and-merge ready 👍

B4nan commented 3 years ago

Looking at that commit I made and the linter passed just fine on that, weird. Maybe we should really disable that rule, personally I am not a big fan of airbnb style guide anyway :D

Or fix it now in the JS files, but the extensions should not be there once its TS.

vladfrangu commented 3 years ago

Looking at that commit I made and the linter passed just fine on that, weird. Maybe we should really disable that rule, personally I am not a big fan of airbnb style guide anyway :D

Or fix it now in the JS files, but the extensions should not be there once its TS.

I'll fix the errors in this PR, you peeps can talk about the eslint rule part 😄

vladfrangu commented 3 years ago

Scratch that, turns out the errors are caused by the fact the files are now .ts instead of .js, which the rule dislikes... 🙃 And we cannot import .ts files, since they don't exist at runtime... Should I just disable the rule for now in the module instead?

B4nan commented 3 years ago

yeah just add ignore comments i guess, we can deal with that later.