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

Make ExecuteJavascriptMiddleware handle requests #264

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Fadarrizz
Copy link

@Fadarrizz Fadarrizz commented Mar 16, 2025

This PR aims to close #239. By letting the middleware handle a request instead of a response and setting the response on the request, we ensure the request is only sent once.

Besides the fact that handling requests instead of responses saves a request, it also make sense for this middleware to handle the request instead of the client, since the request is send with the middleware.

One could say that a separate client should be created for sending Javascript requests and one would have a point in my honest opinion, but it's quite difficult to create Php responses from Browsershot/Puppeteer. Therefore, the changes in this PR offer a middleground.

@Fadarrizz Fadarrizz requested a review from ksassnowski as a code owner March 16, 2025 17:30
@ksassnowski
Copy link
Contributor

I see your point and the implementation looks fine to me. I still have to check if skipping the client could have any unintended consequences for users. I don't think it does since the Downloader still calls onResponseReceived for requests that had their response set by one of the middlewares.

@ksassnowski
Copy link
Contributor

Please rebase your branch on top of the current main branch. That should get rid of most of the pipeline errors. Don't worry about the commitlint action. I can change the commit message when merging.

@Fadarrizz Fadarrizz force-pushed the javascript-middleware-request-handling branch from 6d622b5 to 9aaeac8 Compare March 23, 2025 19:59
@Fadarrizz
Copy link
Author

Thanks for taking the time to look at this PR, @ksassnowski.

I agree with your point and I don't think it will pose any issues for users. The only case I can think of is users that rely on both the client and the middleware, but I can't see why someone would need this.

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.

Requests with ExecuteJavascriptMiddleware are sent twice
2 participants