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

Support computed property keys #252

Closed
robz opened this issue Feb 8, 2015 · 25 comments
Closed

Support computed property keys #252

robz opened this issue Feb 8, 2015 · 25 comments

Comments

@robz
Copy link
Contributor

robz commented Feb 8, 2015

6to5 supports computed properties:

var c = {[1+1]: 2};

But flow produces an error for this: "computed property keys not supported". Any way to get around that error?

@dead-claudia
Copy link

This is actually an ES6 feature. I would consider this as a sub-bug of #62.

@gabelevi
Copy link
Contributor

Yeah, this shouldn't be too hard to fix. Basically just need to decide how computed properties should affect the object type and implement it here. We can't statically know what the property key will be, so my guess is that we can use computed properties to infer the indexer types. So something like

var x: { [key: string]: number }  = { ["hello"]: 123 };

@AprilArcus
Copy link
Contributor

Is that really a concern? Property keys are always coerced to strings, and the computed property syntax doesn't apply to the new ES6 Map/WeakMap collections where keys can be of diverse types.

@rileylark
Copy link

FWIW I had a simple case that I couldn't get flow to ignore, something like:

var eventHandlers = {
  [Actions.DELETE_ITEM]: deleteItem
}

I couldn't get flow to stop giving me an error on this, so I transformed it to:

var eventHandlers = {};
eventHandlers[Actions.DELETE_ITEM] = deleteItem;

Obviously I'm not getting any type checking on eventHandlers but at least flow is checking Actions.DELETE_ITEM and not giving me any errors.

@dead-claudia
Copy link

Object maps with string keys are always far faster than Maps, anyways, even natively implemented ones. And there are only three types you should ever pass to a computed property name: string | symbol | number. If typeof doesn't return either of the first two, then ES2015 requires it to be coerced to a string. (Engines usually optimize array-like accesses, and bail on computed string indices)

// Basic syntax
let obj = { [foo]: bar };

// Equivalent semantics
type KeyType = string | symbol;
let _object = {};
_setProperty(_object, foo, bar);
obj = _object;

function _setProperty(obj: {[key: KeyType]: any}, key: any, value: any) {
  if (typeof key !== 'string' && typeof key !== 'symbol') key = String(key);
  obj[key] = value;
}

// Typical engine's implementation
type KeyType = string | number | symbol;

let _object = {};
%setPropertyInline(_object, foo, bar);
obj = _object;

function %setPropertyInline(obj: {[key: KeyType]: any}, key: any, value: any) {
  if (!%isInteger(key) && typeof key !== 'symbol') {
    %deoptArrayLike(obj); // pseudo-operation
    if (typeof value !== 'string') {
      key = %ToString(key);
    }
  }
  obj[key] = value;
}

@samwgoldman samwgoldman changed the title use with 6to5: computed property keys not supported Support computed property keys Jun 24, 2015
@brentvatne
Copy link

@gabelevi @samwgoldman - any progress on this one?

@samwgoldman
Copy link
Member

No progress, but I think we need more discussion about how to type objects with not-statically-knowable keys. I think Gabe's recommendation of inferring an indexer type is a good one, but there are some existing issues with indexer types, notably #187.

Furthermore, I think we could do a better job on this one if we tackled it after const support lands, because we will be able to know infer more statically, and fall back to indexer types in fewer scenarios.

@abritinthebay
Copy link

Minor thought - wouldn't a warning rather than an error make sense here?

It's, after all, not really a problem with the code so much as it is a statement "hey, we can't work this out atm. Here be dragons."

It will naturally go away when Flow supports them.

@samwgoldman
Copy link
Member

Hmm. Unlike a compiled language, you can still run JavaScript that fails to type check. In a way, all errors are warnings. You can even silence the errors/warnings using the supress_comment configuration. See the test for that feature for an example of its use.

@abritinthebay
Copy link

True, I just mean that semantically it's not an error in the code that Flow is checking so... seems odd to claim it is. Flow hasn't found an error - it just has a problem due it's feature set.

Warnings vs Errors is a very common linter and compiler distinction so it would make a certain amount of sense.

But yes, there's obviously a workaround with the supress_comment config

@dbrock
Copy link

dbrock commented Aug 5, 2015

Could { ["foo"]: bar }, as a special case, be treated equivalently to { foo: bar }?

@samwgoldman
Copy link
Member

I am working on a small part of this, which covers literal strings and const-declared variables. Possibly I will include some inference to detect de-facto const bindings (#482), but I haven't dug into it.

@felixakiragreen
Copy link

👍 for using with redux:

case REQUEST_POSTS:
    return Object.assign({}, state, {
      [action.reddit]: posts(state[action.reddit], action)
    });

@steelbrain
Copy link

Bump

@cristian-sima
Copy link

cristian-sima commented Aug 9, 2016

Any update on this ? It's been year since it was opened

@brentvatne
Copy link

@thejameskyle ;)

@jamiebuilds
Copy link
Contributor

@brentvatne I really need this... refined-github/refined-github#273 (comment)


Brought up possibly doing something like this with @samwgoldman in the future to be a little bit more strict, but that would add yet another concept to object types. So just noting this for history.

declare function bar(): string;

var foo = { [bar()]: true };

if (foo.baz) { let baz: boolean = foo.baz }
// valid

if (foo.baz) { let baz: string = foo.baz }
//                      ^^^^^^ string incompatible with boolean (true)

let baz = foo.baz;
//            ^^^  property `baz`. Property not found in


// multiple computed props:
{ [bar()]: true, [baz()]: 3 }; // { [key: string]: boolean | number }

I'm going to close this issue because they are supported to a sensible degree right now.

@zertosh
Copy link
Member

zertosh commented Aug 18, 2016

@thejameskyle, what about this:

/* @flow */

class Foo {
    [Symbol.iterator](){}
}
4:  [Symbol.iterator](){}
    ^ computed property keys not supported

@jamiebuilds
Copy link
Contributor

Maybe a separate issue is best for that #2286 since everyone in here is discussing objects.

Also, symbols are a separate issue entirely as they aren't supported anywhere yet.

@mkellyclare
Copy link

@thejameskyle

I'm trying to typecheck a simple computed property but flow seems to be missing an error.

I would expect this code - Try Flow Link - to catch the type error with returning a string instead of a number. Is there any way to get flow to catch this when using computed properties?

type Terms = {
    [key:string]: number
}

function terms(name: string):Terms {
    return {
      [name]: 'Not a number'
    }
}

Thanks.

@justin-calleja
Copy link

Does the current level of support for computed property names work for class methods (prototype methods)?

e.g. Using flow-bin version 0.37.4 will error out with "computed property keys not supported" for:

const METHOD_NAMES: { [id:string]: string } = {
  findItem: 'findItem'
};

class X {
    [METHOD_NAMES.findItem]() {
        console.log(`in ${METHOD_NAMES.findItem}`);
    }
}

@heypiotr
Copy link

Run into a similar issue @mkellyclare did, and also while trying to narrow the problem down, discovered there seems to be something weird going on with type aliases as well:

type MyKeyType = string;
type MyValueType = number;
type MyMap = { [MyKeyType]: MyValueType };

let dynamicKey1 = '123';
let myMap1: MyMap = {
  [dynamicKey1]: 'bazinga'
};

let dynamicKey2: MyKeyType = '123';
let myMap2: MyMap = {
  [dynamicKey2]: 'bazinga'
};

Try Flow

myMap1 throws an error, but myMap2 doesn't.

@adamterlson
Copy link

@mkellyclare Did you ever find a work-around for this? This issue seems to persist in 0.53.0.

@mkellyclare
Copy link

@adamjernst
No, haven't been able to work around this, I opened a separate issue for the specific case here but that's still open too. Was trying to type elastic search query objects, but this issue makes it impossible.

@SavePointSam
Copy link

SavePointSam commented Feb 1, 2018

Finally ended up here after looking through a variety of issues (#252 #560 #3258 #4310).

It appears the current state of things is to throw in a // $FlowFixMe. I have also been unable to locate any timeline or effort to resolve this. It's important to note I'm not bunching in Symbol support that is described in some of these places. I would like this rather common pattern to be supported, please.

Abstract ES6+ Example:

const buildObjectWithKey = (key, value) => ({
  [key]: value 
});

const newObject = buildObjectWithKey('foo', false);

React-based Example:

type Props = {
  label: string,
  isActive?: boolean,
  activeClassName?: string,
};

const NavigationItem = ({ label, isActive, activeClassName }: Props) => (
  <button
    className={classNames({ [activeClassName]: activeClassName && isActive })}
  >
    {label}
  </button>
);

Is this something being actively worked on? If not, could someone provide a bit of guidance where to start working on this problem? It appears to have been hanging around for quite some time and I'd be happy to take a look at resolving this issue given a good place to start looking.

Thank you!!

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

No branches or pull requests