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

[Reviewed] Add AdvancedHTTP extension #941

Merged
merged 3 commits into from
Aug 25, 2023
Merged

[Reviewed] Add AdvancedHTTP extension #941

merged 3 commits into from
Aug 25, 2023

Conversation

arthuro555
Copy link
Member

An advanced HTTP client with supports for headers, cache control, built-in opt-in CORS proxy (hosted on the edge via Cloudflare Workers).

Everything has been thoroughly tested via a mock server.

All events can be seen in this example screenshot:

HZ0pDSgQww

@arthuro555 arthuro555 added ✨ New extension A new extension 🏁 Extension almost ready: final fixes The extension is almost ready and needs final fixes and/or polishing ⌨ JavaScript Uses JavaScript code, and thereby needs a reviewer who knows JavaScript. 🔍 Reviewed extension An extension that is to be reviewed in great detail before merging. labels Jul 18, 2023
@arthuro555 arthuro555 requested review from a team as code owners July 18, 2023 21:57
@AlexandreSi
Copy link
Contributor

I have not had a look to the content yet but I saw this in your screenshot:
image

Even for developers, I think that setting the query string parameters directly in a URL is not a good practice.
Seeing how you built the action, I guess it could be easy to have an action such as:

Set the parameter to template Mew key: a, value 1 (set permanently in the template yes/no)

And you could use URLSearchParams behind the scenes. Wdyt?

@arthuro555
Copy link
Member Author

Even for developers, I think that setting the query string parameters directly in a URL is not a good practice.

I was thinking that people would be able to use my already existing URL Tools extension to mutate the URL query string parameters and that it would be a bit out of scope to reimplement all the URL mutating work in this extension.

@tristanbob
Copy link
Contributor

@arthuro555 can you provide an example project that I can use for testing?

@4ian
Copy link
Collaborator

4ian commented Jul 28, 2023

Do you want to go for community while a review is on going?

@arthuro555
Copy link
Member Author

I'm good, I'll wait for the review 👍 I'd rather keep the flexibility of being able to have breaking changes if the review requires some.

@arthuro555
Copy link
Member Author

@tristanbob Here's the example I used in the screenshot: Fetch.zip

I believe I used all conditions, expressions and actions in there.

Copy link
Contributor

@tristanbob tristanbob left a comment

Choose a reason for hiding this comment

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

I have reviewed this extension and chatted with @arthuro555 about it.
The concerns I had were:

  1. If CORS bypass is enabled, requests go through a middle server (by design) and could be a privacy issue. I think this concern is addressed by the text in the "Enable CORS bypass" action, and that CORS bypass is opt-in (not enabled by default).

  2. If CORS bypass is enabled and the service goes down, it could cause an outage for games. This scenario is unlikely since the service is provided by Cloudflare workers.

Thanks @arthuro555 for once again providing high-quality power tools for advanced GDevleop users!

extensions/reviewed/AdvancedHTTP.json Outdated Show resolved Hide resolved
Co-authored-by: Tristan Rhodes <[email protected]>
@arthuro555 arthuro555 merged commit 066b423 into main Aug 25, 2023
1 check passed
@arthuro555 arthuro555 deleted the advanced-http branch August 25, 2023 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏁 Extension almost ready: final fixes The extension is almost ready and needs final fixes and/or polishing ⌨ JavaScript Uses JavaScript code, and thereby needs a reviewer who knows JavaScript. ✨ New extension A new extension 🔍 Reviewed extension An extension that is to be reviewed in great detail before merging.
Projects
Status: Added to GDevelop
Development

Successfully merging this pull request may close these issues.

4 participants