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

Changed property descriptor for Array.includes polyfill #1134

Merged
merged 3 commits into from Oct 4, 2017

Conversation

zwug
Copy link
Contributor

@zwug zwug commented Oct 4, 2017

What kind of change does this PR introduce?
Changed the way of adding Array.includes polyfill for node.js <6.

Did you add or update the examples/?
No need to.

Summary

This is a fix for #1133.

Does this PR introduce a breaking change?

No

@jsf-clabot
Copy link

jsf-clabot commented Oct 4, 2017

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Oct 4, 2017

Codecov Report

Merging #1134 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1134   +/-   ##
=======================================
  Coverage   71.58%   71.58%           
=======================================
  Files           5        5           
  Lines         468      468           
  Branches      151      150    -1     
=======================================
  Hits          335      335           
  Misses        133      133
Impacted Files Coverage Δ
lib/polyfills.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a7f26b...a67d527. Read the comment docs.

lib/polyfills.js Outdated
@@ -5,5 +5,11 @@

// internal-ip@2.x uses [].includes
if (!Array.prototype.includes) {
Array.prototype.includes = require('array-includes');
const includes = require('array-includes');
Object.defineProperty(Array.prototype, 'includes', {
Copy link
Contributor

Choose a reason for hiding this comment

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

would probably be better to use includes.shim(); here, no? https://github.com/ljharb/array-includes/blob/master/shim.js

@zwug
Copy link
Contributor Author

zwug commented Oct 4, 2017

It seems like there is no need for if (!Array.prototype.includes) { because shim checks it anyway

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.

None yet

3 participants