Make ExecuteJavascriptMiddleware handle requests#264
Make ExecuteJavascriptMiddleware handle requests#264Fadarrizz wants to merge 1 commit intoroach-php:mainfrom
Conversation
|
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 |
|
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. |
6d622b5 to
9aaeac8
Compare
|
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. |
|
@ksassnowski The failed checks don't have anything to do with this PR, do they? Can you help me out? |
|
Yeah the failing pipeline seems to be related to a PHPStan upgrade. Don't worry about that for now, I'll fix that on the main branch after this gets merged. I'll still have to do a final test run before merging this but it looks mostly good to me. Sorry for the delay! |
|
No worries and thanks for taking the time. If I can help in any way, let me know! |
9aaeac8 to
16e2dbc
Compare
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.