Skip to content

Conversation

@MQ37
Copy link
Contributor

@MQ37 MQ37 commented Jul 22, 2025

Export internals used in apify-mcp-server repo to prevent importing from dist/.

related to #102

@github-actions github-actions bot added the t-ai Issues owned by the AI team. label Jul 22, 2025
@MQ37 MQ37 requested a review from jirispilka July 22, 2025 13:20
Copy link
Collaborator

@jirispilka jirispilka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think it would be better to expose only the ActorsMcpServer class.
Otherwise, it becomes too easy to break something.

Do we really need to export all these functions?
Could we include them as part of the ActorsMcpServer class instead?

@MQ37
Copy link
Contributor Author

MQ37 commented Jul 23, 2025

I still think it would be better to expose only the ActorsMcpServer class. Otherwise, it becomes too easy to break something.

Do we really need to export all these functions? Could we include them as part of the ActorsMcpServer class instead?

We can include the actorNameToToolName and parseInputParamsFromUrl functions in the ActorsMcpServer class, however I don't think it makes sense to include tool related "config", like categories, in this class. But I think in a long run we should get rid of this class to make the architecture more scalable and less isolated in the class.

@MichalKalita what do you think?

@MichalKalita
Copy link
Contributor

@MQ37 @jirispilka
The library should export all classes, functions, and constants required to work with this library. (It's not happening now)
On the other hand, lib should respect semver, but it's hard in this case.

Exporting non-stable parts in /internals is a good compromise for me.

I don't know if some other NPM package uses this library as a dependency. https://www.npmjs.com/package/@apify/actors-mcp-server?activeTab=dependents
Almost all users use it to start a local MCP server, not as a dependency in other NPM modules.

TLDR: I'm approving this PR

@MichalKalita
Copy link
Contributor

Copy link
Collaborator

@jirispilka jirispilka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, exporting more tools will just make things harder for us.
Once something is exposed, we shouldn’t change it, or it might break in apify-mcp.

Let's approve it and we will see.

@MQ37
Copy link
Contributor Author

MQ37 commented Jul 24, 2025

For the record, exporting more tools will just make things harder for us. Once something is exposed, we shouldn’t change it, or it might break in apify-mcp.

Let's approve it and we will see.

To some degree I agree but since its marked as internal and we are building that for us only and not for the public (I mean the /internals export path) it should be fine to do breaking changes since it is only used in single repo.

Merging 👍

@MQ37 MQ37 merged commit e1f529d into master Jul 24, 2025
2 checks passed
@MQ37 MQ37 deleted the fix/export-internals branch July 24, 2025 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-ai Issues owned by the AI team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants