Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

no-object-literal-type-assertion rule in Reduce #4333

Closed
zheeeng opened this issue Dec 2, 2018 · 10 comments
Closed

no-object-literal-type-assertion rule in Reduce #4333

zheeeng opened this issue Dec 2, 2018 · 10 comments

Comments

@zheeeng
Copy link

zheeeng commented Dec 2, 2018

Rule Suggestion

Is your rule for a general problem or is it specific to your development style?
It is general that constructing an array/object from empty array and object.

What does your suggested rule do?
Allow use assertion in reduce function by default

List several examples where your rule could be used

    const groupMap = activities.reduce(
        (m, act) => {
          m[act.date] = m[act.date] ? [...m[act.date], act] : [act]

          return m
        },
        // tslint:disable-next-line:no-object-literal-type-assertion
        {} as { [key: string]: Activity[] },
      )

Additional context

@JoshuaKGoldberg
Copy link
Contributor

Hey @zheeeng, thanks for posting this! Could you please post a code snippet that includes the definitions of the Activity type/interface and the type of the activities variable? I'm finding it hard to get your sample to compile.

@zheeeng
Copy link
Author

zheeeng commented Dec 4, 2018

@JoshuaKGoldberg

There is a sample:

const arr = ['foo', 'bar']

const map = arr.reduce(
  (m, key) => {
    m[key] = 42

    return m
  },
  {} as { [key: string]: number },
)

tslint reports: [tslint] Type assertion on object literals is forbidden, use a type annotation instead. [no-object-literal-type-assertion]

It is a common way in reduce but now we have to suppress the lint reporting every time.

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Dec 5, 2018

In this case the rule is correct, though. You don't actually need that type assertion. The code compiles fine in TypeScript and gives no object-literal-type-assertion errors if you just remove the cast:

const arr = ['foo', 'bar']

const map = arr.reduce(
  (m, key) => {
    m[key] = 42

    return m
  },
  {}
)

Do you have an example of the showing a complaint when the cast is necessary?

@zheeeng
Copy link
Author

zheeeng commented Dec 5, 2018

@JoshuaKGoldberg We lost the accurate type of map, it is {}.

Thought we have a workaround that:

const arr = ['foo', 'bar']

const map = arr.reduce<{ [key: string]: number }>(
  (m, key) => {
    m[key] = 42

    return m
  },
  {},
)

But when we encount Generic, we have to use assertion:

function Test<O extends { value: number }> (o: O) {

  const arr = [42, 43]

  // { value: 5 } is not assignable to generic O
  const ret =  arr.reduce<O>(
    (sum, num) => {
      sum.value = o.value + num + o.value

      return sum
    },
    // have to use assertion there
    { value: 5 } as O,
  )

  return ret
}

@ghost
Copy link

ghost commented Jan 13, 2019

Hey,
If I take your example mentioned above,

const arr = ['foo', 'bar']

const map = arr.reduce(
  (m, key) => {
    m[key] = 42 // error, no index signature.ts(7017)

    return m
  },
  {} /* as {[key:string]: any} */,
)

Without the cast, I get

Element implicitly has an 'any' type because type '{}' has no index signature.

, as suppressImplicitAnyIndexErrors in the ts compiler is disabled by default (and thats good).
I think, it would be useful to relax this rule for object literals, that just define/cast their index signature inline in a concise manner.

@adidahiya
Copy link
Contributor

@ford04 in that case, you should use @zheeeng's suggestion of specifying the function signature with arr.reduce<{[key:string]: any}>

however, for the more complex generic case, @zheeeng is right and the rule is currently not well-suited to deal with that code pattern. Open to PRs to fix this.

@karol-majewski
Copy link
Contributor

I can take a stab at this. I have a question though: if we are to allow type assertions for object literals when they are used as parameters, then should that behavior be enabled by default (just like asserting any or unknown) or should that be an option?

@JoshuaKGoldberg
Copy link
Contributor

Hey @karol-majewski, sorry for taking so long to respond! I'm inclined to prefer not enabling this by default, since in most cases this behavior isn't desirable. But that can certainly be revisited if it's found to be common / very useful?

@karol-majewski
Copy link
Contributor

Done in #4521.

@adidahiya
Copy link
Contributor

tentatively declaring this fixed by #4521, feel free to provide more feedback once that's released

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

No branches or pull requests

4 participants