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

Guard against prototype extended arrays #769

Merged

Conversation

davidenglishmusic
Copy link
Contributor

Certain frameworks like Ember extend the array type with additional properties. Although they can be disabled ( https://guides.emberjs.com/release/configuring-ember/disabling-prototype-extensions/ ), it would remove some helpful functionality. In the case, the iteration through the additional properties by the execute validator code can result in undefined being passed as the third argument. The code change guards against that case.

@tagliala
Copy link
Contributor

Thanks!

The PR cannot be merged because, according to codeclimate, it overcomplicates the executeValidators method: https://codeclimate.com/github/DavyJonesLocker/client_side_validations/pull/769

Anyway... we have others for ... in in the source code, do we need to replace them too?

image

@tagliala
Copy link
Contributor

@davidenglishmusic thanks.

What about other for .. in ?

@davidenglishmusic
Copy link
Contributor Author

@tagliala thanks for the quick responses here. I am going to test out a few of the others to see if any additional changes are necessary and will post back.

@tagliala
Copy link
Contributor

@davidenglishmusic

If this can cause issues, and some eslint configurations forbid the use of for ... in I'm fine with a JavaScript standard loop with all the variables and stuff

@davidenglishmusic
Copy link
Contributor Author

davidenglishmusic commented Sep 18, 2019

The guard clause has resolved the issue I was experiencing and mirrors the approach in the executeValidators method ( https://github.com/DavyJonesLocker/client_side_validations/blob/master/src/main.js#L156 ). It could be worthwhile moving away from the for ... in as you said although it strikes me that it might take a lot of data structure conversion.

@tagliala
Copy link
Contributor

@davidenglishmusic I will proceed to merge and release during the weekend, thanks 👍

@tagliala tagliala merged commit f3b6d13 into DavyJonesLocker:master Sep 21, 2019
@tagliala tagliala added this to the 16.0.2 milestone Sep 21, 2019
@tagliala
Copy link
Contributor

@davidenglishmusic thanks

16.0.2 gem / 0.0.5 npm released

If you have more time to invest on CSV, there are some features I'm looking for help:

  1. Idempotent events
  2. Run JavaScript tests on CI (Run javascript unit tests on CI #591)
  3. Promise-based approach (client_side_validations uses synchronous ajax #226) (Proposal: change to model based validations on client #490)
  4. Remove jQuery dependency

Ref: #692

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants