Skip to content

fix: unhandled exception for index not found in reflected types#45

Open
yamishav wants to merge 3 commits into
epiphone:masterfrom
yamishav:yoni/hotfix
Open

fix: unhandled exception for index not found in reflected types#45
yamishav wants to merge 3 commits into
epiphone:masterfrom
yamishav:yoni/hotfix

Conversation

@yamishav

Copy link
Copy Markdown

Fix for:
On

const { explicitType, index, object, method } = param;
const type = Reflect.getMetadata('design:paramtypes', object, method)[index];

An error can be thrown if the index is not in the array;

@codecov

codecov Bot commented Apr 26, 2020

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.48%. Comparing base (addba0b) to head (65f04c2).
⚠️ Report is 74 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #45   +/-   ##
=======================================
  Coverage   99.47%   99.48%           
=======================================
  Files           4        4           
  Lines         191      193    +2     
  Branches       58       59    +1     
=======================================
+ Hits          190      192    +2     
  Misses          1        1           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@epiphone epiphone left a comment

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.

Thanks! Can you add a test case to demonstrate the error?

Also please run yarn format so that we get rid of the whitespace changes in the diff.

@yamishav

yamishav commented Apr 27, 2020

Copy link
Copy Markdown
Author

Hi, I updated the yarn format.
Regarding the test, we are having a complex senior where we auto-generate the endpoints so it's a little complex to reproduce the bug on a test
In this case, the reflection has issues handling the getMetadata and the index is trying to access a out of index in the array

@epiphone

Copy link
Copy Markdown
Owner

Ok, I'll look into adding a test and getting this merged his week.

@yamishav

Copy link
Copy Markdown
Author

any updates?

@yamishav

Copy link
Copy Markdown
Author

Hi, Any chance we can move on with the PR?

@epiphone

Copy link
Copy Markdown
Owner

Okay, please revert the lock file deletion and we'll get this merged!

@epiphone

Copy link
Copy Markdown
Owner

The reason I'm a bit hesitant merging this is it's unclear to me whether this is something we should handle in the library or is this an edge case specific to your particular setup and thus better handled in your codebase. Hard to say without knowing the context or having a test!

Then again it should be a pretty non-intrusive change so might as well go for it, I think.

@yamishav

Copy link
Copy Markdown
Author

ok. I have reverted it

@epiphone

Copy link
Copy Markdown
Owner

Your previous commit added a bunch of .idea/ files, let's get rid of those eh?

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.

2 participants