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

fix: support all input fields as query params in standby (mainly proxy) #59

Merged
merged 4 commits into from
Mar 19, 2025

Conversation

metalwarrior665
Copy link
Member

I tested this locally. I think we could get rid of the Readme section about query params and just send users to https://apify.com/apify/rag-web-browser/input-schema so we don't have duplicate documentation.

Copy link
Collaborator

@matyascimbulka matyascimbulka left a comment

Choose a reason for hiding this comment

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

I think it makes sense to support all of the input parameters. There is just a small issue with duplicate code which produces duplicate log messages.

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.

Good point about removing query parameters from the README—it will simplify things.

I think we should only provide examples for non-obvious ones, such as outputFormats. Let’s handle this in another PR. I’ve created an issue for it: #62

I believe Matyáš meant removing the checkAndRemoveExtraParams function, as it is no longer needed.

Copy link
Collaborator

@matyascimbulka matyascimbulka left a comment

Choose a reason for hiding this comment

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

The issue is fixed, thank you.

@matyascimbulka
Copy link
Collaborator

I believe Matyáš meant removing the checkAndRemoveExtraParams function, as it is no longer needed.

At first I did but looking at it closer now I can see that these functions aren't identical. The checkAndRemoveExtraParams function iterates over the defaults object and this change iterates over the inputSchema.properties. And these objects aren't identical (the defaults object has extra properties for validation purposes).

So I guess this begs a bigger question whether we need the defaults object when we can read the input schema directly. What do you think?

@metalwarrior665
Copy link
Member Author

@matyascimbulka Good catch. Fixed to iterate all defaults.

@jirispilka I removed the checkAndRemoveExtraParams but the logic of parsing is now a bit more complex.

This got quite complex for such a small change (that I need for one customer) so before any further updates, I would do a larger refactor to make future updates easier. I would start with looking at the input, we could officially deprecate some fields (e.g. keepAlive seems redundant with Standby) and unify the types with input schema and query params, I think we can just ensure that Standby and input are the same.

Copy link
Collaborator

@matyascimbulka matyascimbulka left a comment

Choose a reason for hiding this comment

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

I agree that the larger refactor for the input handling and types around it would be useful. But lets not block this PR for that.

@jirispilka
Copy link
Collaborator

Ok, thank you. Yeah, it seems a bit complicated now.

@metalwarrior665 metalwarrior665 merged commit 9327510 into master Mar 19, 2025
1 check passed
@metalwarrior665 metalwarrior665 deleted the fix/standby-proxy-config-input branch March 19, 2025 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants