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

Catch unexpected values in array #63

Merged
merged 2 commits into from Jul 16, 2018
Merged

Catch unexpected values in array #63

merged 2 commits into from Jul 16, 2018

Conversation

@lesliedoherty
Copy link
Contributor

lesliedoherty commented Jul 2, 2018

As a result of a brief linkedin bug where the array prototype was modified, this simple solutions offers better defensive detection of values passed in the config by checking that the value has its own property associated with it.

screenshot 2018-07-02 15 13 57

@lesliedoherty

This comment has been minimized.

Copy link
Contributor Author

lesliedoherty commented Jul 3, 2018

Note regarding failing tests. I've opened an issue: #64

@lesliedoherty lesliedoherty requested review from angrytoast and iamEAP Jul 3, 2018
@iamEAP

This comment has been minimized.

Copy link
Contributor

iamEAP commented Jul 3, 2018

Thanks for the issue on the test fails @lesliedoherty. I can repro them locally too, but they only occur on the git1 and git2 versions of jQuery.

Maybe we stop testing against "dev" branches for jQuery, but at least update the versions we test against? e.g.

Go from

['1.3.2', '1.4.4', '1.5.2', '1.6.4', '1.7.2', '1.8.3', '1.9.1', '1.10.2', '1.11.3', '1.12.0', 'git1', '2.0.3', '2.1.4', '2.2.0', 'git2']

...to...

['1.3.2', '1.4.4', '1.5.2', '1.6.4', '1.7.2', '1.8.3', '1.9.1', '1.10.2', '1.11.3', '1.12.4', '2.0.3', '2.1.4', '2.2.4', '3.0.0', '3.1.1', '3.2.1']

In Gruntfile.js?

Copy link
Member

angrytoast left a comment

The changeset looks good. Would recommend going with @iamEAP's comment in #63 (comment)

@iamEAP

This comment has been minimized.

Copy link
Contributor

iamEAP commented Jul 16, 2018

Thanks all. I updated the jQuery test targets. Merging to master, then picking the change into branches where it applies and releasing updates.

@iamEAP iamEAP merged commit 3c835b0 into master Jul 16, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@iamEAP iamEAP deleted the patch branch Jul 16, 2018
@iamEAP iamEAP mentioned this pull request Jul 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.