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

HTTP Function Refactor #641

Open
wants to merge 10 commits into
base: v6
Choose a base branch
from
Open

HTTP Function Refactor #641

wants to merge 10 commits into from

Conversation

Asayukiii
Copy link
Contributor

@Asayukiii Asayukiii commented Aug 17, 2024

Following my suggestion, here are my changes.

Refactors

$httpRequest

The headers, error and property fields were deleted. Current usage is:

$httpRequest[url;method?;body?]

This function will create a property called "http" in d.data where all request info will be saved.

Additions

$httpAddHeader

Adds a header to be used in the request.
Usage:

$httpAddHeader[name;value]

Example

$httpRequest[https://api.com/submit;POST;{
    "key": "value"
}]
$httpAddHeader[content-type;application/json]

$httpClearOptions

Clears the data that a past request must have cached.
Usage:

$httpClearOptions

Example

$httpRequest[...]

$httpClearOptions
$httpResult[key;key]
$httpRequest[...]

$httpGetContentType

Retrieves the response content type.
Usage:

$httpGetContentType

Example

$sendMessage[$if[$httpGetContentType==application/json;Response is JSON;Response is any]]
$httpRequest[...]

$httpGetHeader

Retrieves a response header.
Usage:

$httpGetHeader[name]

Example

$httpGetHeader[x-cors-auth]
$httpRequest[...]

$httpOk

Retrieves whether request has a success code (boolean).
Usage

$httpOk

Example

$if[$httpOk==true;API response with success;Something went wrong]
$httpRequest[...]

$httpRemoveHeader

Removes a header from the HTTP data.
Usage:

$httpRemoveHeader[name]

Example

$httpRequest[...]

$httpRemoveHeader[auth]
$httpRequest[...]
$httpAddHeader[auth;sklakdlasmdlmksd]

$httpResult

Retrieves a result from the HTTP request.
Usage:

$httpResult[...properties?]

Example

$httpResult[users;0;firstName]
$httpRequest[...]

$httpStatus

Returns the status of the HTTP response.
Usage:

$httpStatus

Example

$checkCondition[$httpStatus==200] $comment[true]
$httpRequest[...]

$httpWasRedirected

Returns whether HTTP request was redirected to another endpoint/site.
Usage:

$httpWasRedirected

Example

$log[HTTP redirected? => $httpWasRedirected] $comment[true]
$httpRequest[$get[url];POST]
$let[url;http://www.growthbook.io/test]

Tests

bot.addCommandType("ready", {
    code: `
        $log[Content Type should be "text/plain": $checkCondition[$httpGetContentType==text/plain]]
        $httpRequest[$get[url]]
        $let[url;https://raw.githubusercontent.com/aoijs/website/main/src/content/docs/functions/addSelectMenu.md]
        $c[^^^ Test 4 starts here]

        $log[HTTP html result: $httpResult]
        $log[HTTP res content type should be "text/html": $checkCondition[$httpGetContentType==text/html]]
        $httpRequest[$get[url]]
        $let[url;https://aoi.js.org]
        $c[^^^ Test 3 starts here]

        $log[content type header should not exist: $checkCondition[$httpGetContentType==]]
        $httpClearOptions
        $log[content type header should exist: $checkCondition[$httpGetContentType!=]]
        $log[Status code should be "201": $checkCondition[$httpStatus==201]]
        $log[HTTP result should return an user Id and should be a number: $httpResult[id] | $isInteger[$httpResult[id]]]
        $httpRequest[$get[url];post;{
            "firstName": "Asayuki",
            "lastName": "whocares",
            "age": "1975"
        }]
        $httpAddHeader[content-type;application/json]
        $let[url;https://dummyjson.com/users/add]
        $c[^^^ Test 2 starts here]

        $httpClearOptions
        $log[HTTP redirected is false: $checkCondition[$httpWasRedirected==false]]
        $log[HTTP OK is true: $checkCondition[$httpOk==true]]
        $log[HTTP res content type should be "application/json": $checkCondition[$httpGetContentType==application/json]]
        $log[Status code should be "200": $checkCondition[$httpStatus==200]]
        $log[User's first name should be "Emily": $checkCondition[$httpResult[firstName]==Emily]]
        $httpRequest[$get[url]]
        $let[url;https://dummyjson.com/users/1]
        $c[^^^ Test 1 starts here]
    `
})

Any comments, ideas about this are welcome!

This comment was edited by a maintainer to make it easier to scroll to the comments.

@Faf4a
Copy link
Member

Faf4a commented Aug 17, 2024

Hello, first of all, thanks for the pull request! 😃

However, this would be a breaking change therefore it would be better to allow users to also use the old behaviour of httpRequest for the time being. (should be fairly simple to implement)

Which then can be removed in a future version (v6.9 probably) to give users more time to update their codes.

The code changes look fine on a first look, I don't see any specific issues, I'll review them properly once I get home.

@Faf4a Faf4a added state: pending Approval must be done first, to be accepted. type: function functions related aoi.js this issue/Pr is related to aoi.js labels Aug 17, 2024
@NezitX
Copy link

NezitX commented Aug 17, 2024

Hey, I've got some feature suggestions:

Since this is a breaking changes and you're saving HTTP request in the d.data object, why not allow multiple requests like $httpsRequest[name;...]? This would let users make advanced requests, like connecting to multi data requests, and use $httpOk[name?] or any other function.

Also, I think renaming $httpClearOptions to $httpClose or something similar would make more sense.

@Asayukiii
Copy link
Contributor Author

Hey, I've got some feature suggestions:

Since this is a breaking changes and you're saving HTTP request in the d.data object, why not allow multiple requests like $httpsRequest[name;...]? This would let users make advanced requests, like connecting to multi data requests, and use $httpOk[name?] or any other function.

Also, I think renaming $httpClearOptions to $httpClose or something similar would make more sense.

Sounds kinda good, but you can save all HTTP results in variables and objects.
Anyway, if implemented, name field may be the last field to make it less breaking:

$httpRequest[url;method?;body?;name?]

@Kino7916
Copy link
Contributor

Hey, these are some good commits you got in store for aoi.js!

However I do want to point out some of my opinions that could be changed in your pull request.

  • Misleading behaviour or $httpAddHeader. The code instead adds the object property if it is not present and changes the existing header value. Might consider changing the name to $httpSetHeader.
  • Your example of $httpWasRedirected is misleading, it demonstrates the use of $httpStatus instead of the function.
  • There's no need for $httpGetContentType, it is already accessable with $httpGetHeader[Content-Type]
  • Maybe add a function to indicate the reason for the range of status codes. As example: 400 is Bad Request which is a Client Request Error.
  • More static functions like $httpURL[...], $httpRequestBody[...] and etc.

src/functions/httpStatus.js Outdated Show resolved Hide resolved
@Asayukiii
Copy link
Contributor Author

Asayukiii commented Aug 22, 2024

Okay... I followed some suggestion. Mostly of them given by Kino.

Changes

- $httpAddHeader
+ $httpSetHeader

- $httpWasRedirected
+ $httpRedirected

- $httpRequest[url;method?;body?]
+ $httpRequest[url;method?;body?;encoding?]

- $httpGetContentType
  • I added an encoding field to $httpRequest to handle the result in case the response is a buffer.

I am still wondering if add "named requests" because you can easily save HTTP request data in objects.

@USERSATOSHI
Copy link
Member

I think you can go for name request @Faf4a what do you think?

@Faf4a
Copy link
Member

Faf4a commented Sep 12, 2024

Agreeing with ya

@USERSATOSHI
Copy link
Member

@Asayukiii hi, You can move forward with the named request

@Asayukiii
Copy link
Contributor Author

Done, I made the following changes.

+ $httpSetBody[body;name?]
+ $httpSetCredentials[credentialsText;name?]

- $httpClearOptions
+ $httpClearOptions[name?]

- $httpGetHeader[headerName]
+ $httpGetHeader[headerName;name?] 

- $httpOk
+ $httpOk[name?]

- $httpRedirected
+ $httpRedirected[name?]

- $httpStatus
+ $httpStatus[name?]

- $httpRemoveHeader[headerName]
+ $httpRemoveHeader[headerName;name?]

- $httpSetHeader[headerName;headerValue]
+$httpSetHeader[headerName;headerValue;name?]

- $httpRequest[url;method?:body?;encoding?]
+ $httpRequest[url;method?:body?;encoding?;name?]

- $httpResult[..properties?]
+ $httpResult[name?:...properties?]

Tested with the following code.

client.readyCommand({
    code: `
        $log[REQUEST_3_SUMMARY: {
            redirected: $httpRedirected[buffer],
            success: $httpOk[buffer],
            status: $httpStatus[buffer],
            result: buffer stuff,
            contentType: $httpGetHeader[content-type;buffer]
        }]
        $httpRequest[https://dummyjson.com/image/150;GET;;utf-8;buffer]

        $log[REQUEST_2_SUMMARY: {
            redirected: $httpRedirected[login],
            success: $httpOk[login],
            status: $httpStatus[login],
            result: $httpResult[login],
            contentType: $httpGetHeader[content-type;login]
        }]
        $httpRequest[https://dummyjson.com/auth/login;POST;;;login]
        $httpSetHeader[Content-Type;application/json;login]
        $httpSetCredentials[include;login]
        $httpSetBody[{
            "username": "emilys",
            "password": "youshallnotpass"
        };login]

        $log[REQUEST_1_SUMMARY: {
            redirected: $httpRedirected,
            success: $httpOk,
            status: $httpStatus,
            result: $httpResult,
            contentType: $httpGetHeader[content-type]
        }]
        $httpRequest[https://jsonplaceholder.typicode.com/todos/1;GET]
    `
})

@Faf4a
Copy link
Member

Faf4a commented Sep 19, 2024

Will check later. cc @supremesupreme you too please thanks

@Faf4a Faf4a added state: blocked PR needs changes before further review and removed state: pending Approval must be done first, to be accepted. labels Sep 24, 2024
@Faf4a
Copy link
Member

Faf4a commented Sep 24, 2024

LGTM, not approving as it'd be better to not include it in 6.9 but rather a later version and announcing the breaking change first.

@Leref
Copy link
Member

Leref commented Oct 13, 2024

Resolve conflicts, additionally it looks good. This a major breaking change, definitely next update.

@Faf4a Faf4a added this to the 6.10.0 milestone Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aoi.js this issue/Pr is related to aoi.js state: blocked PR needs changes before further review type: function functions related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants