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 errors array concatenation #10

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mdnfiras
Copy link
Contributor

@mdnfiras mdnfiras commented Mar 19, 2022

When no error is present, combinedErrors will be equal to [ [ ] , [ ] , ..... , [ ] ] (array of empty arrays), and therefore the check for errors with combinedErrors.length > 0 will always be true (combinedErrors.length = number of inside arrays) and will trigger the denial of the request.

This PR will fix how error strings are concatenated so that combinedErrors will be equal to [ ] when no error is present, and ["error 1", "error 2", .... , "error n"] when errors are present.

the issue is described more here #9

@@ -1,5 +1,5 @@
export function denyOnErrors(...errors: Array<string[]>) {
const combinedErrors: string[] = [].concat.apply(errors)
const combinedErrors: Array<string> = Array<string>().concat.apply(Array<string>(), errors)

Choose a reason for hiding this comment

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

Changing [].concat.apply(errors) to [].concat.apply([], errors) should be enough

Copy link
Contributor Author

@mdnfiras mdnfiras Mar 22, 2022

Choose a reason for hiding this comment

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

npm run compile produces errors with const combinedErrors: string[] = [].concat.apply([], errors)

running in the node:10 container:

Screenshot from 2022-03-22 17-00-55

> @jspolicy/jspolicy-typescript-starter@0.0.1 compile /work
> tsc && npm run bundle

src/util/helpers.ts:2:58 - error TS2345: Argument of type 'string[][]' is not assignable to parameter of type 'ConcatArray<never>[]'.
  Type 'string[]' is not assignable to type 'ConcatArray<never>'.
    The types returned by 'slice(...)' are incompatible between these types.
      Type 'string[]' is not assignable to type 'never[]'.
        Type 'string' is not assignable to type 'never'.

2     const combinedErrors: string[] = [].concat.apply([], errors)
                                                           ~~~~~~


Found 1 error.

npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! @jspolicy/jspolicy-typescript-starter@0.0.1 compile: `tsc && npm run bundle`
npm ERR! Exit status 2
npm ERR! 
npm ERR! Failed at the @jspolicy/jspolicy-typescript-starter@0.0.1 compile script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /root/.npm/_logs/2022-03-22T15_57_55_787Z-debug.log

Copy link
Contributor Author

@mdnfiras mdnfiras Mar 22, 2022

Choose a reason for hiding this comment

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

@baracoder however, const combinedErrors: string[] = Array<string>().concat.apply([], errors) also worked, should I write this instead?

EDIT: force pushed to implement const combinedErrors: string[] = Array<string>().concat.apply([], errors) instead

Choose a reason for hiding this comment

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

Sorry, I should have tested it in the actual code instead of Chrome's console 😅
Found an alternative though: errors.flat(), see MDN

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@baracoder yup, a newer PR also solves this same issue using errors.flat()
#28

@semmet95
Copy link

semmet95 commented Nov 19, 2023

I ran into the same issue. I know nothing about TypeScript so took me half a day to realize I wasn't the one messing things up. I initialized combinedErrors as following and that worked out for me:

const combinedErrors: string[] = errors.reduce((accumulator, value) => accumulator.concat(value), []);

I hope they merge this PR soon.

@merusso
Copy link

merusso commented Mar 22, 2024

#28 also fixes this, by using errors.flat()

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.

None yet

4 participants