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

[fix] encoding is still required in TS even though it has default value from co-body #155

Closed
3 tasks done
JeromeDeLeon opened this issue Jul 10, 2023 · 1 comment · Fixed by #160
Closed
3 tasks done
Assignees
Labels

Comments

@JeromeDeLeon
Copy link

Describe the bug

Node.js version: 18.6.1
OS version: macOS 12.6

Description: When I used the v5 of this lib, I am getting a type error from the encoding field being required but the README.md says that it has a default value suggesting it isn't required.

- **encoding**: requested encoding. Default is `utf-8` by `co-body`.

Another thing is that encoding is not being passed on to the co-body's parse function so it seems useless compared to v4 where it uses copy-to's copy function to merge options.

bodyparser/index.js

Lines 50 to 111 in 5678a79

const jsonOpts = formatOptions(opts, 'json');
const formOpts = formatOptions(opts, 'form');
const textOpts = formatOptions(opts, 'text');
const xmlOpts = formatOptions(opts, 'xml');
const extendTypes = opts.extendTypes || {};
extendType(jsonTypes, extendTypes.json);
extendType(formTypes, extendTypes.form);
extendType(textTypes, extendTypes.text);
extendType(xmlTypes, extendTypes.xml);
// eslint-disable-next-line func-names
return async function bodyParser(ctx, next) {
if (ctx.request.body !== undefined || ctx.disableBodyParser)
return await next(); // eslint-disable-line no-return-await
try {
const res = await parseBody(ctx);
ctx.request.body = 'parsed' in res ? res.parsed : {};
if (ctx.request.rawBody === undefined) ctx.request.rawBody = res.raw;
} catch (err) {
if (onerror) {
onerror(err, ctx);
} else {
throw err;
}
}
await next();
};
async function parseBody(ctx) {
if (
enableJson &&
((detectJSON && detectJSON(ctx)) ||
isTypes(ctx.request.get('content-type'), jsonTypes))
) {
return await parse.json(ctx, jsonOpts); // eslint-disable-line no-return-await
}
if (enableForm && ctx.request.is(formTypes)) {
return await parse.form(ctx, formOpts); // eslint-disable-line no-return-await
}
if (enableText && ctx.request.is(textTypes)) {
return (await parse.text(ctx, textOpts)) || '';
}
if (enableXml && ctx.request.is(xmlTypes)) {
return (await parse.text(ctx, xmlOpts)) || '';
}
return {};
}
};
function formatOptions(opts, type) {
const res = {};
copy(opts).to(res);
res.limit = opts[type + 'Limit'];
return res;
}

compared to v5 where it limits the options

const parserOptions = {
// force co-body return raw body
returnRawBody: true,
strict: bodyType === 'json' ? restOpts.jsonStrict : undefined,
[`${bodyType}Types`]: mimeTypes[bodyType],
limit: restOpts[`${shouldParseBodyAs('xml') ? 'xml' : bodyType}Limit`],
};
return parser[bodyType](ctx, parserOptions) as Promise<
Record<string, string>
>;

so I'm not entirely sure if we should document this as breaking changes that we're not able to override the encoding or any other values or fix this type of issue.

Also, I think it's an issue with co-body type declaration where they explicitly put undefined as part of the type telling us to explicitly pass a value of undefined

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/def5996adb3596c8c468922ed0ac55110cd47fcc/types/co-body/index.d.ts#L33-L42

Actual behavior

Argument of type '{ jsonLimit: string; }' is not assignable to parameter of type 'BodyParserOptions'.
  Property 'encoding' is missing in type '{ jsonLimit: string; }' but required in type 'Pick<Options, "encoding">'.ts(2345)

Expected behavior

Should not be getting any type of error

Code to reproduce

For whatever reason, I wasn't able to replicate it in the TS playground but I was able to replicate it in CodeSandbox.

Workaround

bodyParser({ ...yourOptions, encoding: undefined }) // only if you're not overriding it

Checklist

  • I have searched through GitHub issues for similar issues.
  • I have completely read through the README and documentation.
  • I have tested my code with the latest version of Node.js and this package and confirmed it is still not working.
@3imed-jaberi 3imed-jaberi self-assigned this Apr 7, 2024
@3imed-jaberi
Copy link
Member

Thanks @JeromeDeLeon for your catch! Yup, it looks like an issue with the co-body, and from our side we don't pass the encoding option ... I am working on it and expect the new release soon!

titanism pushed a commit that referenced this issue Apr 8, 2024
## Description

Fix #155 and #157

## Checklist

- [x] I have ensured my pull request is not behind the main or master
branch of the original repository.
- [x] I have rebased all commits where necessary so that reviewing this
pull request can be done without having to merge it first.
- [x] I have written a commit message that passes commitlint linting.
- [x] I have ensured that my code changes pass linting tests.
- [x] I have ensured that my code changes pass unit tests.
- [x] I have described my pull request and the reasons for code changes
along with context if necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants