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

Update body after beforeRequest #1453

Merged
merged 7 commits into from Sep 18, 2020
Merged

Update body after beforeRequest #1453

merged 7 commits into from Sep 18, 2020

Conversation

Giotino
Copy link
Collaborator

@Giotino Giotino commented Sep 10, 2020

This PR makes possibile updating the body in a beforeRquest hook

Example:

await got.post({
  json: {payload: 'old'},
  hooks: {
    beforeRequest: [
      options => {
        options.body = JSON.stringify({payload: 'new'});
        options.headers['content-length'] = options.body.length.toString();
      }
    ]
  }
});

This provides a "low level" access to the body, as can be seen in the example there's still the need to manually update headers and other properties affected by the body.

Actually there's no way to access the final body computed by Got, meaning that a user can't find anywhere in the options parameter the body generated by _finalizeBody(). This could give us a chance to implement breforeRequest hook as follow:

beforeRequest(options: NormalizedOptions, computedValues: ComputedValues);

where computedValues contains computed values (who would have guessed... :) ) from Got such as the final body.

At this point the user should edit the computedValues insted of the options in order to alter the request behavior.

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.

Fixes #1408
Fixes #1427

@Giotino
Copy link
Collaborator Author

Giotino commented Sep 15, 2020

@szmarczak @sindresorhus Any feedback on this?

@sindresorhus
Copy link
Owner

sindresorhus commented Sep 15, 2020

@Giotino Don't forget to mark the PR as "Ready for review" when you're done with all changes from your side.

@sindresorhus
Copy link
Owner

Does this need any docs changes or additions? If the capability is not already documented, we should do so.

@Giotino
Copy link
Collaborator Author

Giotino commented Sep 15, 2020

@Giotino Don't forget to mark the PR as "Ready for review" when you're done with all changes from your side.

Of course, but this PR is not yet ready for review.

The things that I want to discuss are:

  • The computedValues (see PR comment)
  • The ability to change options.json and have it reflected on the request body (this is going to be an higher level of access to the body, I don't recommend it)

@szmarczak
Copy link
Collaborator

Actually there's no way to access the final body computed by Got, meaning that a user can't find anywhere in the options parameter the body generated by _finalizeBody(). This could give us a chance to implement breforeRequest hook as follow:

I strongly disagree. When I rewrote Got, I meant this to be used only internally. Why anyone would want to access the computed body anyway?

@szmarczak
Copy link
Collaborator

szmarczak commented Sep 15, 2020

The ability to change options.json and have it reflected on the request body (this is going to be an higher level of access to the body, I don't recommend it)

I agree on this but this would require re-normalization, which wouldn't be efficient. Not sure if the code would look good though.

I've been thinking of another way to normalize options, I'll let you know when I have a draft of it.

Can't we [for now] just mention that overriding options.form and options.json is unsupported?

@Giotino
Copy link
Collaborator Author

Giotino commented Sep 16, 2020

Can't we [for now] just mention that overriding options.form and options.json is unsupported?

I think we can go with this, also changing the form and json could be done easily using only the body and the headers (see PR message).

Today (or tomorrow) I'm going to add the documentation and then it should be ready for review.

@szmarczak
Copy link
Collaborator

Regarding the computed body thing, why do you need it? I can't think of any example, just asking :)

@Giotino
Copy link
Collaborator Author

Giotino commented Sep 17, 2020

Regarding the computed body thing, why do you need it? I can't think of any example, just asking :)

I have no real life example, but if someone want to apply some modification before the request that depends on the body, or maybe they want only to do something with the computed body (maybe log it).

I'm not saying that it's a "must have" but I'm exploring some possibilities, currently I don't think anyone is using it as that, but never say never. Maybe we could implement the feature like it's now on the PR write body (only options.body) and then add the other features on request (if some people ask for that).

@Giotino Giotino marked this pull request as ready for review September 18, 2020 10:19
@szmarczak szmarczak merged commit e1c1844 into sindresorhus:master Sep 18, 2020
@Giotino Giotino deleted the beforeRequest-body branch September 18, 2020 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants