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

Array.prototype methods cause validation error in the expected transp… #125

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

Conversation

thirug010
Copy link

added condition to skip Array.Prototype method irritated as item/object in the array for issue "#123"

…orts[<method>] to be 'string'

added condition to skip Array.prototype method irritated as item/object  in the array
@codecov-commenter
Copy link

codecov-commenter commented Apr 7, 2023

Codecov Report

Patch coverage: 33.33% and project coverage change: -0.06 ⚠️

Comparison is base (aef3754) 92.95% compared to head (1af4825) 92.89%.

❗ Current head 1af4825 differs from pull request most recent head f634222. Consider uploading reports for the commit f634222 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #125      +/-   ##
==========================================
- Coverage   92.95%   92.89%   -0.06%     
==========================================
  Files          16       16              
  Lines        5992     5998       +6     
==========================================
+ Hits         5570     5572       +2     
- Misses        422      426       +4     
Impacted Files Coverage Δ
lib/parser.js 88.55% <33.33%> (-0.42%) ⬇️
lib/validator.js 92.06% <33.33%> (-0.24%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JamesCullum
Copy link
Member

Thanks a lot for your contribution - can you give an example of what you were trying before that didn't work, but works now with your code?

Maybe you can add this as test, so that we can future-proof it?

@thirug010
Copy link
Author

Thanks a lot for your contribution - can you give an example of what you were trying before that didn't work, but works now with your code?

Maybe you can add this as test, so that we can future-proof it?

For my nodejs app we have few prototype function for ease of use like contains(), pushRange() ..., because of usage of 'in' to iterate the elements of array it consider the methods also as elements to iterate and fails the validation such as transports is not type of string.

Example: Array.prototype.contains =function(str) {.....} , Array.prototype.pushRange =function(items) {}
var tempArray = ['usb','other','internal'] when array has two above prototype methods added,
for(let a in tempArray ) show total of 5 item including the 3 elements and 2 porotype methods.

I will add the test cases for the same.

@wparad
Copy link
Contributor

wparad commented Apr 13, 2023

This is confusing and adds unnecessary bloat to the library. Adding things here to account for individual use cases that are incorrectly implemented decreases the value being provided by the library.

I'm going to recommend that if you create the poly fills appropriately to exclude the functions from the array iterator in your implementation code, as it does not belong here.

@thirug010
Copy link
Author

thirug010 commented Apr 14, 2023

This is confusing and adds unnecessary bloat to the library. Adding things here to account for individual use cases that are incorrectly implemented decreases the value being provided by the library.

I'm going to recommend that if you create the poly fills appropriately to exclude the functions from the array iterator in your implementation code, as it does not belong here.

ideally for..of need to be used to iterates over the values of an iterate-able object. instead of for..in iterates over all enumerable property keys of an object.

for..of is used in almost of the place to iterate object of array in the code, expect while iterating through allowCredentials, transports only.

using for..of to iterate allowCredentials, transports cause more test cases to fail, so I gone with for..in with filtering out the methods.

I believe array.protyotype methods are very common to use, so I thought adding this would help.

@JamesCullum
Copy link
Member

JamesCullum commented Apr 14, 2023

I agree to some extent with both of you. If "for of" is the correct syntax, I think one needs to look closer at why the other tests are failing after a change - this would imply that either "for in" is the better choice or there are some hard wired dependencies.

I'm not really sure about your fix though - you are just checking if it is a function, and then skipping most validations etc. This might be dangerous to users who are accidently passing a function and are relying on everything to work properly.

My understanding of the use case would be that if it is a function, it should execute it and validate the result as if it was a value - is my understanding correct?

And coverage shouldn't decrease, so we'd always need tests for new things.

@thirug010
Copy link
Author

"I'm not really sure about your fix though - you are just checking if it is a function, and then skipping most validations etc. This might be dangerous to users who are accidently passing a function and are relying on everything to work properly"

==> Yes I agree, this is not correct, I am checking the code / test cases so this issue can be fixed fully with for...of

"My understanding of the use case would be that if it is a function, it should execute it and validate the result as if it was a value - is my understanding correct?"
==> Yes

"And coverage shouldn't decrease, so we'd always need tests for new things"
==> we need to adjust / update the current test case (or code) if need and need to add test cases to cover for..of with method

@JamesCullum JamesCullum added the waiting Waiting on author label Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in discussion waiting Waiting on author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants