Skip to content
This repository has been archived by the owner on Feb 26, 2022. It is now read-only.

bug 1092231 - fix traits to work with Symbols #1694

Merged
merged 1 commit into from Nov 4, 2014

Conversation

zombie
Copy link
Member

@zombie zombie commented Nov 3, 2014

No description provided.

@@ -90,3 +89,10 @@ function omit(source, ...values) {
return copy;
}
exports.omit = omit;

// get both object property Names and Symbols
function getOwnPropertyKeys(object)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is misleading name, specially since we do have Object.keys, how about something like:

const getMemberNames = (object, options={keys:true, names:false, symbols:false}) => {
  const symbols = options.symbols && Object.getOwnPropertySymbols(object);
  const names = options.names ? Object.getOwnPropertyNames(object) :
                options.keys ? Object.keys(object) :
                null;

  return symbols && names ? [...names, ...symbols] :
         symbols ? symbols :
         names;
}

That way caller can configure what to include and what to exclude & name is less confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

after some discussion in #jsapi and reading the es6 spec (draft), i'm gonna disagree with this.

a "Key" is a commonly used term thru-out the es6 spec to signify both String and Symbol-valued property keys. there is even an internal method with (almost) the same name i proposed [[OwnPropertyKeys]], which is not a coincidence. also, various public-api references to "Keys" as representing both String and Symbol values (in Proxy handler trap, Reflect.ownKeys, etc). the only exception is indeed Object.keys (compat reasons obviously), but that's an inconsistency on the language-level, not one we are introducing with this API. IMHO, using "MemberNames" to represent both String and Symbol values is even more misleading, and goes directly against the spec (term "Names" is never used for non-String values).

and second, regardless of the name, a generalized method like you are suggesting doesn't seem too widely useful imho:

  • it's mixing and sometimes returns only own, and other times all (enumerable, string) property keys. most of the time, at a single point in code, you are either interested in all or only own properties.
  • mixing { keys: true, symbols: true } would return all String-keyed properties, but only own Symbol-keyed ones (i doubt that would ever be useful).
  • it's too verbose way to do any other task (except the one i specifically need in this case). why would you a) import the method from the module, and b) call getMemberNames(obj, { params: true }) in any other case (except this specific one), when every other use case is covered and available as an existing single method call on the Object global.

so, if you still disagree with my method name/definition as a public exposed api, i'm fine with just defining and using it as a local helper function in the two modules that need it.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's mixing and sometimes returns only own, and other times all (enumerable, string) property keys. most of the time, at a single point in code, you are either interested in all or only own properties.

That is not true Object.keys returns all own enumerable property names, while Object.getOwnPropertyNames returns all own enumerable + non-enumerable property names. It's just matter of specifying which one you want to use, maybe indeed having {nonenumerables: true} isntead of names: true would make more sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

mixing { keys: true, symbols: true } would return all String-keyed properties, but only own Symbol-keyed ones (i doubt that would ever be useful).

That is not true, see my previous comment

it's too verbose way to do any other task (except the one i specifically need in this case). why would you a) import the method from the module, and b) call getMemberNames(obj, { params: true }) in any other case (except this specific one), when every other use case is covered and available as an existing single method call on the Object global.

In your case it will be just getMemberNames(object, {symbols:true})

Also I'd be fine with making default {names:true, symbols:true} so you could just use getMemberNames(object).

zombie added a commit that referenced this pull request Nov 4, 2014
bug 1092231 - fix traits to work with Symbols, r=@Gozala
@zombie zombie merged commit c968107 into mozilla:master Nov 4, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants