-
Notifications
You must be signed in to change notification settings - Fork 117
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
Adding support to es6 Set collection #183
Conversation
* Returns true if the given object is a set. | ||
*/ | ||
export const isSet = (object) => | ||
object instanceof Set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check is unreliable across realms (like iframes). instanceof
shouldn't be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd need to cache the getter from Set.prototype.size
, and .call
it in a try
/catch
, in order to reliably detect Set
s.
* indicate a non-match. | ||
*/ | ||
export const setContains = (set, value, compareValues) => | ||
arrayContains([ ...set ], value, compareValues) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a Set - why would we want to reify it as a real array, when it's got .has()
which is O(1)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set#has
is using identity equality (===
) so new Set([ {} ]).has({})
is false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, Set.prototype.forEach.call(set, x => compareValues(value, x))
would be more efficient (but some engines don't have forEach, so you'd need to use Function
and for..of
there)
@@ -1,10 +1,10 @@ | |||
import expect from '../index' | |||
|
|||
describe('toExclude', () => { | |||
it('requires the actual value to be an array or string', () => { | |||
it('requires the actual value to be an array, set, object or string', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think "object" is sufficient here, since a Set is an object.
* Returns true if the given object is a set. | ||
*/ | ||
export const isSet = (object) => | ||
Object.prototype.toString.call(object) === '[object Set]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is fakeable since ES6 has Symbol.toStringTag
- Object#toString
output is not reliable >= ES6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what what would you think be better? object[Symbol.toStringTag] === 'Set'
or object[Symbol.iterator] !== undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is only one possible solution, which I mentioned earlier
it('does not throw when an object contains an expected object with an unexpected value', () => { | ||
expect(() => { | ||
expect({ a: 1 }).toExclude({ a: 2 }) | ||
}).toNotThrow() | ||
}) | ||
|
||
it('does not throw when an array does not contain the expected value', () => { | ||
it('does not throw when an string does not contain the expected value', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/an string/a string
Hi, Any idea what should be done? |
If there's a bug in |
(the object-inspect bug is filed here, and it turns out it's a bug in core-js/babel-polyfill) |
@@ -55,7 +55,7 @@ module.exports = (config) => { | |||
reporters: [ 'mocha' ], | |||
|
|||
files: [ | |||
'node_modules/babel-polyfill/dist/polyfill.js', | |||
'node_modules/es6-shim/es6-shim.js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw since this is JS, this can probably do path.relative(require.resolve('es6-shim'), process.cwd())
or something similar
object-inspect has a workaround for core-js's broken Map/Set implementation in v1.3.0, and separately, core-js fixed it in v2.5.0. |
This package is no longer being updated; v21+ is maintained here |
No description provided.