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

Request: Remove the some type restrictions on Computed Properties syntax. #17942

Closed
zheeeng opened this issue Aug 21, 2017 · 13 comments
Closed
Labels
Duplicate An existing issue was already created

Comments

@zheeeng
Copy link

zheeeng commented Aug 21, 2017

TypeScript Version: 2.4.0

Code

In ES6 Computed Properties syntax, we are allowed to use any type as the property of an object. e.g.:

const s = Symbol()

// `[ts] A computed property name must be of type 'string', 'number', 'symbol', or 'any'.`
const awesomeObj = {
    [() => {}]: 'function',
    [null]: 'null',
    [undefined]: 'undefined',
    [s]: 'symbol',
}

console.log(Object.keys(awesomeObj)) // ["() => {}", "null", "undefined"]

Above are valid usages, but TS tips me: [ts] A computed property name must be of type 'string', 'number', 'symbol', or 'any'.

Another non-sense restriction is we can't set symbols or any other types exclude string and number as object's indexes while we defining object interface.

--del--

// `An index signature parameter type must be 'string' or 'number'.`
interface IAwesomeObj {
    [key: any]: number
}

--del--

--updated--

// `An index signature parameter type must be 'string' or 'number'.`
interface IAwesomeObj {
    [key: Symbol]: number
}

--updated--

The key will eventually be string or symbol, but TS should allow us defining any types as the properties.

@zheeeng
Copy link
Author

zheeeng commented Aug 21, 2017

In redux-actions, there is a usage like:

const noop = createAction('INCREMENT');

const reducer = handleActions({
  [noop]: (state, action) => ({
    counter: state.counter + action.payload
  })
}, { counter: 0 });

noop is a function, and its toString mehtod will return string 'noop', all seems right but in TS we get errors.
To workaround, I have to:

const reducer = handleActions({
  [noop as any]: (state, action) => ({
    counter: state.counter + action.payload
  })
}, { counter: 0 });

Hope a TypeScript official solution.

@aluanhaddad
Copy link
Contributor

They should all be invalid because they are pointlessly bad code that offers no benefits.

Each one of those values that you rightly point out are tolerated as computed keys in JavaScript are, with the exception of symbols, converted into Strings.

@aluanhaddad
Copy link
Contributor

Your example from the second post should work though, and I'm pretty sure it used to

@zheeeng
Copy link
Author

zheeeng commented Aug 21, 2017

@aluanhaddad I updated the second example, It should support Sybmol keys

@zheeeng
Copy link
Author

zheeeng commented Aug 21, 2017

@aluanhaddad In redux-actions, we use the created action function as the key, and this action function implements that it's toString method return its Action type, so actually this action function refers to an string.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 21, 2017

In redux-actions, we use the created action function as the key, and this action function implements that it's toString method return its Action type, so actually this action function refers to an string.

That sort of magical implicit coercion is exactly why JavaScript has a bad name for itself. That sort of bad coding practice is exactly what TypeScript is for. Why try to fight it?

The Symbols as a index type is a duplicate of #1863

@zheeeng
Copy link
Author

zheeeng commented Aug 21, 2017

@kitsonk I agree with you that it's a discouraged tricky in a way. But, in redux-actions usage, I think it's naturally reusing the action function itself as its reducer identification. Maybe a modification can be done to eliminate smell by making the action function takes its Action Type as a property. But someway somehow there always be someone would love to employ implicit toString method calling in Computed Property syntax, because of the javascript just has the magic.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 21, 2017

But someway somehow there always be someone would love to employ implicit toString method calling in Computed Property syntax, because of the javascript just has the magic.

Just because you can, doesn't mean you should.

Especially with functions, .toString() is really a dangerous identifier, really dangerous. There is nothing to block you from passing in a non-reducer function that doesn't have the custom code to generate a meaningful identifier and instead what you get is the whole function written out to a string.

Using one ES6+ feature (computed properties) to create a Map, when ES6 introduced the Map construct which can use the object reference as a key seems totally non-sensical and really really dangerous.

There are whole classes of things that are possible in JavaScript that TypeScript errors on because they are dangerous or non-sensical constructs, and my personal opinion is not to make TypeScript more flexible to accommodate these anti-patterns.

@zheeeng
Copy link
Author

zheeeng commented Aug 21, 2017

@kitsonk Thx for your opinions and I get many thoughts. Just in that implements toString is under controlled, altering on function's prototype doesn't affect the behaviour of that usage, am I'm missed some other situations?

@svieira
Copy link

svieira commented Aug 21, 2017

You can already do the "indexable function" type with the existing affordances:

type FWithString<T extends Function> = T & string;

function createAction<T extends string>(name: T): FWithString<() => {type: T}> {
  const a = () => ({ type: name });
  a.toString = () => name;
  return a as any as FWithString<() => {type: T}>;
}

const noop = createAction('NOOP');

// You can call it
const a = noop();

// You can also index with it
const handler = {
  [noop]: 123
}

@aluanhaddad
Copy link
Contributor

Just to add a little bit to @kitsonk's point about it being poor coding practice, there are libraries and even ECMAScript feature implementations like Array.isArray that rely on .toString behavior. If we were talking about Arrays for example, this would break.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 21, 2017

Duplicate of #5579

@mhegazy mhegazy marked this as a duplicate of #5579 Aug 21, 2017
@mhegazy mhegazy added the Duplicate An existing issue was already created label Aug 21, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Sep 5, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mhegazy mhegazy closed this as completed Sep 5, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

5 participants