Skip to content

Fixes AwilixResolutionError throwing TypeError if resolution stack contains symbols.#205

Merged
jeffijoe merged 2 commits intojeffijoe:masterfrom
astephens25:master
Oct 23, 2020
Merged

Fixes AwilixResolutionError throwing TypeError if resolution stack contains symbols.#205
jeffijoe merged 2 commits intojeffijoe:masterfrom
astephens25:master

Conversation

@astephens25
Copy link
Copy Markdown
Contributor

I use symbols heavily in resolution so they may exist on multiple levels. I found the AwilixResolutionError was throwing another error: "TypeError: Cannot convert a Symbol value to a string". The symbol "names" in the resolution stack were failing on the array join. I added a test to cover this scenario.

Code coverage: 100%
Linting Passes

Please let me know if there is anything else you would like me to do.

@jeffijoe
Copy link
Copy Markdown
Owner

jeffijoe commented Oct 23, 2020

Thanks for the PR!

I think this will fail if you have a symbol and a regular string where the symbol name and the string are the same since you toString when adding to the resolution stack. I think what you really want is to do it in the resolution stack path creation.

EDIT: Hmm weird, a few lines above I already do name = name.toString() if its a symbol 🤔 So really this shouldn't change anything?

EDIT2: Nevermind, I am blind 😂

src/errors.ts Outdated
}
resolutionStack = resolutionStack.slice()
resolutionStack = resolutionStack
.slice()
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Since you do a map you can remove slice.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Total face-palm on my part. I just pushed that change.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 2540f97 on astephens25:master into 994ed7b on jeffijoe:master.

1 similar comment
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 2540f97 on astephens25:master into 994ed7b on jeffijoe:master.

Copy link
Copy Markdown
Owner

@jeffijoe jeffijoe left a comment

Choose a reason for hiding this comment

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

Thanks!

@jeffijoe jeffijoe merged commit 1c7c4db into jeffijoe:master Oct 23, 2020
@jeffijoe
Copy link
Copy Markdown
Owner

Released in v4.2.7 🥳

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.

3 participants