Monorepo rewrite

[B4nan]

As @mnmkng mentioned, would be good to split this into multiple packages and convert the repo to monorepo, so we do not need to use deep imports (which is in general a bad practice). It will also serve as a PoC for future monorepos like the SDK.

Proposal

There are several options, mostly dependent on package manager (e.g. we could use pnpm or yarn that truly support workspaces), but in the end we will still probably need lerna too. It will help mainly with release management and build orchestration.

Here is the proposed set of repositories (partially based on usage, partially on 3rd party dependencies):

Unfortunately we can't use package names like apify-shared/consts (forbidden by NPM), so it will have to be something like @apify-shared/consts.

• @apify-shared/consts
  • consts.js
  • regexs.js
• @apify-shared/log
  • log.js
  • log_consts.js
  • log_helpers.js
  • logger.js
  • logger_json.js
  • logger_text.js
• @apify-shared/input_schema
  • input_schema.js
  • input_schema.json
  • intl.js
• @apify-shared/git
  • git.js
• @apify-shared/salesforce_client
  • salesforce_client.js
• @apify-shared/datastructures
  • linked_list.js
  • list_dictionary.js
  • lru_cache.js
• @apify-shared/markdown
  • markdown_renderers.js
  • marked.js
• @apify-shared/utilities
  • exponential_backoff.js
  • health_checker.js
  • image_proxy_client.js
  • parse_jsonl_stream.js
  • streams_utilities.js
  • utilities.client.js
  • utilities.js
  • webhook_payload_template.js

Happy to hear suggestions about this, as I haven't seen much of the internal repos so this is mostly based on the SDK. This will have direct impact on breaking changes, so if there is a file that is worth having as a single file repository just because it is used extensibly, we should indeed do it that way, especially if its in client facing code.

We might also want to split some of the large files (e.g. the consts.js one is quite large).

Suggestions

TypeScript

Personally I would also use this opportunity to convert the repository to typescript. We already added automatic generation of types recently, but they are all based purely on JS inference, so not great in general. Also I do have a working setup of lerna with TS so it would actually be easier for me to set it up that way.

Removal of default exports

Another thing I don't like is the overusage of default exports. They just add complexity to the compiled code, and also reduces DX (find usage & go to declarations are buggy as they jump to the import line instead of the actual implementation, at least in WS). If we are fine with making some additional breaking changes, I'd suggest getting rid of them in most places (haven't checked much code, I could understand the one in log, if we need to export an instance, default export sounds like the right thing to do).

Removal of polyfills

The only present polyfill is startsWith - I believe its already the time when its safe to say ES6 is supported everywhere, so this should not be needed. Unless we have actual need for this, let's get rid of it.

Removal of underscore

It's the ES6 time, most of the helper methods are natively supported or easily implemented on very few LoC. Mostly used ones that are clearly not needed: isArray, mapObject, map, some, filter, extends (as per the docs its the same as Object.assign(), it's not recursive), also pretty much all of the is* methods can be replaced with single line variants (either typeof or instanceof). I'd probably keep it in the salesforce client package, as there it is used heavily, but most of the other usages are easy to replace.

Btw if you think underscore is a small dependency, it's not (also native methods will be always faster):

➜  apify-shared-js git:(master) ✗ npx howfat underscore
Dependencies: 0
Size: 880.22kb
Files: 503

Yarn workspaces

Personally a big fan of yarn (v1), using it for years and never did me wrong, and it makes a good tandem when combined with lerna. NPM recently added support for workspaces too, but from what I read, it's far from complete. Here is some reading about why it is good combination:

https://doppelmutzi.github.io/monorepo-lerna-yarn-workspaces/

With lerna we should be fine with NPM too (as it was supported way even before NPM added workspaces), so this is really just an open question. But as I mentioned above, I do have a working version of this and it also uses yarn workspaces, so doing it this way would ease the implementation for me (or better - remove some unknowns - it could be also as easy as with yarn).

Also worth reading: https://hyperfoo.io/posts/npm-7-workspaces-1

Alternative

We might also just add index.js file and reexport things from there, and instead of accessing apify-shared/consts we could do import { consts } from 'apify-shared'.

[B4nan]

cc @mtrunkat

[fnesveda]

If we're gonna do this major refactoring, I think we should also take a look at what actually needs to be in apify-shared, because it's needed by other public packages, and what is used only by our private packages and should be moved to some private shared package. This could simplify the package structure quite a bit.

I went through all the public dependants of apify-shared listed on npm and they're only importing from these modules:

apify-shared/consts
apify-shared/input_schema
apify-shared/list_dictionary
apify-shared/log
apify-shared/lru_cache
apify-shared/regexs
apify-shared/streams_utilities
apify-shared/utilities

If I exclude those files and their dependencies, I get these modules which are not used publicly and could be moved to a private package:

exponential_backoff
git
health_checker
image_proxy_client
markdown_renderers
marked
parse_jsonl_stream
salesforce_client
webhook_payload_template

We could also go through the functions in utilities and utilities.client and see which are used in public packages, which ones in private ones, which ones only in a single package, and which are not used at all, and clean those up a bit. There's a lot of functions which in my opinion shouldn't be in a package like this, like isNullOrUndefined or newPromise.

[mtrunkat]

1. There are some things we can just remove as AFAIK they are not used any more:

• exponential_backoff
• salesforce_client
  ...?

2. As Franta says, we can move some classes (HealthChecker, markdown_renderers, ...) to the apify-core repo where we will also have a set of private packages (?managed by Lerna). But having these things public as both repos and NPM packages is nice as we want to support open source. We can also use some interesting issues as intro tasks.

3. NPM workspaces vs Yarn workspaces - if it's not a big deal I'd beware of Yarn as we can't use Yarn internally (because of Meteor) and having to switch between Yarn and NPM is a bit annoying.

[mtrunkat]

4. Underscore - we can get rid of it or replace it with Lodash as Lodash supports tree shaking and also is present basically in the very single repository we have as a dependency of some other package (not sure here about SDK etc.) so it's not adding anything to bundle.

[B4nan]

as we can't use Yarn internally (because of Meteor)

Can you provide more details? I am talking about yarn v1 (not v2, aka berry), so the one that uses regular node_modules folder as npm does. Found one issue about this, didn't go thru the whole thread but it feels like it should be supported.

meteor/meteor#7918 (comment)

I will try with NPM first and see what are the actual problems yarn could solve. From what I read, the workspaces support in NPM is really just an MVP currently, far away from being complete. But as mentioned, it might be as well totally fine as lerna should probably fill the missing parts for us anyway.

Anyway, don't want this to be the main discussion point (although I strongly believe yarn is worth adopting 😉), the important part is to agree on the rest, mainly what should be there and what not and the structure in general. Also whether TS or not, and what about the default exports.

Worth noting that this will be a major bump anyway (we could even go for 1.0.0 rather than 0.8.0), so legacy code can still stay on older version (e.g. for something that uses the things we'd remove).

[mtrunkat]

We were trying to use Yarn in the past as some people get it working although it's not supported officially by Meteor. But for some reason, it resulted in many problems as for example some dependencies had duplicate occurrences in Node modules. This affected the Meteor only not other apps.

BTW Meteor has even exact NPM version bundled with it and does not officially support any other 🤦

[B4nan]

Ok understood, started with the rewrite on friday, so far it looks like things will be just fine regardless the package manager, will be smarter later this week. Btw as I understood, we are planning to get rid of meteor in the long run, right? The more I hear about, the more it gets scary :D

Would be good to agree on what can be removed/moved elsewhere. Currently I just dropped the polyfills file, I am very much aligned with what @fnesveda said, can I get a green light on the list he provided?

• exponential_backoff
• git
• health_checker
• image_proxy_client
• markdown_renderers
• marked
• parse_jsonl_stream
• salesforce_client
• webhook_payload_template

As mentioned earlier, legacy projects can just pin the old version and keep using it from there.

[mnmkng]

I think the best way to do this would be to start with the packages used in the SDK and other public libraries while cleaning up old and obsolete code. For the platform, apify-shared will keep working as is and we can then later migrate all the other code (or not) when the time arises.

[mnmkng]

Also, about the streams_utilities. I think those are now used only in the SDK and in http-request which has now been made obsolete by got-scraping so we can probably move this code to SDK and delete it from here.

[B4nan]

I forgot to mention one thing, we cant use apify-shared/consts etc as package names, so in fact this will not cause any autoupdate issues on upstream, as there will be new set of packages, e.g. @apify-shared/consts.

[mnmkng]

We were thinking of using just @apify/consts and so on. It's quite obvious that the code is shared from the fact that it is published as a package.

[mtrunkat]

Ok understood, started with the rewrite on friday, so far it looks like things will be just fine regardless the package manager, will be smarter later this week. Btw as I understood, we are planning to get rid of meteor in the long run, right? The more I hear about, the more it gets scary :D

Would be good to agree on what can be removed/moved elsewhere. Currently I just dropped the polyfills file, I am very much aligned with what @fnesveda said, can I get a green light on the list he provided?

• exponential_backoff
• git
• health_checker
• image_proxy_client
• markdown_renderers
• marked
• parse_jsonl_stream
• salesforce_client
• webhook_payload_template

As mentioned earlier, legacy projects can just pin the old version and keep using it from there.

Most of these are still used. Only exponential_backoff and salesforce_client are dead. I'd keep them here if it's not a big deal. We could perhaps create a one utility package as these are mostly small utility classes.

[fnesveda]

I'd still move everything that doesn't need to be public to the apify-core monorepo, probably first to apifier-storage and then eventually somewhere better when we're reorganizing that. It'll become much easier to work with, right now updating the markdown renderers for example is a huge pain. Besides, keeping it here will only increase the number of updates to this package and with that the chance that something will break, which would be more painful.

[mtrunkat]

@fnesveda there is one thing I am a bit afraid of - it will be much easier to update these components when you need changes to be used in app/API/daemons (apify-core) but it will be more complicated for anybody who needs to change something and then use it in web/proxy/conductor/worker/... because the release will depend on the release of whole apify-core monorepo which is much slower than releasing apify-shared.