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

Add options to customize parsing/stringifying JSON #1298

Merged
merged 23 commits into from Jun 5, 2020
Merged

Add options to customize parsing/stringifying JSON #1298

merged 23 commits into from Jun 5, 2020

Conversation

Giotino
Copy link
Collaborator

@Giotino Giotino commented May 31, 2020

Custom function to parse JSON responses.

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.
  • If it's a new feature, I have included documentation updates.

Fixes #1274

source/core/index.ts Outdated Show resolved Hide resolved
source/core/index.ts Outdated Show resolved Hide resolved
@szmarczak
Copy link
Collaborator

szmarczak commented May 31, 2020

@Giotino Can you also do stringifyJson? Actually that's not needed as people can pass JSON.stringify(object, stringifier) as a body option. We should keep the complexity minimum.

@szmarczak
Copy link
Collaborator

IMO this is good to merge.

@szmarczak
Copy link
Collaborator

szmarczak commented May 31, 2020

Actually I'm having second thoughts. This also adds more complexity. What's against having

const {body} = await got(...);
const parsed = JSON.parse(body, parser);

I don't think like this change is really necessary.

@Giotino
Copy link
Collaborator Author

Giotino commented May 31, 2020

I thought so, but with parseJson you can have an extended got that always behave the same without having to explicitly use your parser every time.

It's not a "must have" feature, but since someone showed interest in that it means that it might be nice to have. Also I don't think it adds that much complexity.

@Giotino
Copy link
Collaborator Author

Giotino commented May 31, 2020

Maybe we should ear more opinions before proceeding.

readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Show resolved Hide resolved
source/core/index.ts Outdated Show resolved Hide resolved
Giotino and others added 3 commits June 1, 2020 10:33
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
@kirillgroshkov
Copy link

Actually I'm having second thoughts. This also adds more complexity. What's against having

const {body} = await got(...);
const parsed = JSON.parse(body, parser);

In our case we have ~50 methods that call API via got(...).json(). To add a reviver to them right now means adding it to 50 places and risking that we forget to add it to newly added methods. Having it built in the extended got means the change in only 1 place and safety to add more methods.

Same thinking as why got.extend function and hooks exist in the first place. Without it - it'll need copy/pasting the hooks to every usage.

@szmarczak
Copy link
Collaborator

Hmm... @kirillgroshkov is right. So stringifyJson is also welcome, but unnecessary. It's up to you :)

@Giotino
Copy link
Collaborator Author

Giotino commented Jun 2, 2020

Examples needs to be fixed/improved.

@sindresorhus @szmarczak
While working on the documentation I noted that the examples are incosistent.
Examples of examples :)

const got = require('got');

await got();
const got = require('got');

(async () => {
  await got();
})();
await got();

I think that const got = require('got'); and the async wrapper are not needed, I can work on a PR normalizing all the examples.

@szmarczak
Copy link
Collaborator

I think that const got = require('got'); and the async wrapper are not needed

@sindresorhus has said a while ago that these examples should be 100% functional. Copy-paste and it should work.

source/core/index.ts Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

https://github.com/sindresorhus/got/pull/1298/files#r433074774 still needs to be resolved.

@sindresorhus
Copy link
Owner

I think that const got = require('got'); and the async wrapper are not needed, I can work on a PR normalizing all the examples.

That would be great as a separate PR. We should normalize to always require Got and always use the async IIFE.

@Giotino
Copy link
Collaborator Author

Giotino commented Jun 2, 2020

https://github.com/sindresorhus/got/pull/1298/files#r433074774 still needs to be resolved.

Yeah, I'm aware of it, I was kinda busy so I delayed it a bit.

readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
Giotino and others added 6 commits June 4, 2020 14:48
Co-authored-by: Szymon Marczak <36894700+szmarczak@users.noreply.github.com>
Co-authored-by: Szymon Marczak <36894700+szmarczak@users.noreply.github.com>
@sindresorhus sindresorhus changed the title Custom function to parse JSON responses Add options to customize parsing/stringifying JSON Jun 5, 2020
@sindresorhus sindresorhus merged commit cb4da8d into sindresorhus:master Jun 5, 2020
@sindresorhus
Copy link
Owner

Excellent 👌

@Giotino Giotino deleted the issue-1274 branch June 5, 2020 15:46
szmarczak added a commit to jaulz/got that referenced this pull request Jul 6, 2020
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
Co-authored-by: Szymon Marczak <36894700+szmarczak@users.noreply.github.com>
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.

parseJson option
4 participants