Skip to content

feat(eslint-plugin): add option to no-object-literal-type-assertion rule #87

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

Merged
merged 8 commits into from
Jan 22, 2019
Merged

feat(eslint-plugin): add option to no-object-literal-type-assertion rule #87

merged 8 commits into from
Jan 22, 2019

Conversation

armano2
Copy link
Collaborator

@armano2 armano2 commented Jan 20, 2019

This PR adds new option to no-object-literal-type-assertion to disable it when used in call or new expression.

@IlyaSemenov
fixes: #66

@armano2 armano2 requested a review from bradzacher January 20, 2019 06:22
@codecov
Copy link

codecov bot commented Jan 20, 2019

Codecov Report

Merging #87 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #87      +/-   ##
==========================================
+ Coverage   95.08%   95.09%   +<.01%     
==========================================
  Files          49       49              
  Lines        2483     2487       +4     
  Branches      371      371              
==========================================
+ Hits         2361     2365       +4     
  Misses         73       73              
  Partials       49       49
Impacted Files Coverage Δ
...ugin/lib/rules/no-object-literal-type-assertion.js 100% <100%> (ø) ⬆️

@JamesHenry
Copy link
Member

@armano2 Conflicts now just FYI

@armano2
Copy link
Collaborator Author

armano2 commented Jan 20, 2019

i don't like name of option here: allowInCallExpression is not fully ok, this will allow to use type assertion in new and call expression....

@j-f1
Copy link
Contributor

j-f1 commented Jan 20, 2019

allowAsParameter?

@scottohara
Copy link
Contributor

There may be another use case here that needs consideration: return values.

Consider the following code:

// Returns either Thing or Partial<Thing>

function getNewOrExistingThing(id?: number): Thing | Partial<Thing> {

  // if an id was not passed, return a Partial<Thing> (new)
  if (isNaN(id)) return { ... } as Partial<Thing>;

  // otherwise, return a Thing (existing)
  return { ... } as Thing;
}

Unlike the const x = { ... } as T -> const x: T = { ... } case, there is no equivalent workaround for return values.

Would the proposed allowAsParameter option also allow for the return value scenario above, or should there be a allowAsReturn option as well?

@j-f1
Copy link
Contributor

j-f1 commented Jan 21, 2019

Since TypeScript uses a structural type system, the returned object will pass the type check as long as it has all the required properties of Thing or Partial<Thing>.

@JamesHenry
Copy link
Member

JamesHenry commented Jan 21, 2019

@scottohara Following on from @j-f1 comment, I am not sure about your example.

Let's take the example where there is no extra Partial involved for simplicity:

https://typescript-play.js.org/#code/PQ18ZXTMAQDkDyAVOBxJSAiAoXAlgHYAuApgE4BmAhgMZlwoAWxA5nAN65y9xUB7AQC44AIyEAbMjSIBuHnzE0KoiQOmyFAX3xUArkTokCAonDZkSCMgHckFAKIAPAgGcTRNi3YAKAJSiPl5cihRW+hTm3Hx8giJwJBT6ZIracDRuTKxeOvh0Zh6JORwAvBZWNvZOrh7swWwBCriwrW3tHXAAQo4oKI4ASvjE5NT0jA2hsfFqUjLyirzKquJzWri6uAZGJmYV1nYOLu6e3iUATAFBJVNwBURFJNfscOUxsfxCokkpaQq84RIkXMJDyuHujwur32VSOtVODSaQA

I would say that the second form is what you would do - allowing the type assertion is really not desirable in most cases, as it opens you up to potential future issues after refactorings, and so having a linting option to allow it doesn't seem to make sense to me. Please let me know if I am missing something.

In the rare case where you do need the type assertion behaviour as a return value, using an eslint-disable comment is probably a good thing - it shows to other readers of the code (including yourself in the future) that you acknowledge you are overriding linter feedback and helps flag it up as an unusual case.

Let me know what you think

Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

Code LGTM.

We can always add more cases later

@scottohara
Copy link
Contributor

@JamesHenry - thank you for your playground example, and as usual you are 100% spot on.

The second form does indeed (at the cost of one extra const declaration) both avoid the need for a type assertion on the return value, and conveys the developer's intent far better.

@JamesHenry
Copy link
Member

I'll cut a full release after this goes in, we've merged in quite a few fixes since v1 now

@JamesHenry JamesHenry merged commit 9f501a1 into typescript-eslint:master Jan 22, 2019
@armano2 armano2 deleted the no-object-literal-type-assertion branch January 22, 2019 23:50
@armano2
Copy link
Collaborator Author

armano2 commented Jan 22, 2019

@JamesHenry its way easier now to test and fix issues (when we have control over all packages)

@JamesHenry
Copy link
Member

Awesome, that was the hope :) I am very happy with how our monorepo is shaping up!

I've released https://github.com/typescript-eslint/typescript-eslint/releases/tag/v1.1.0

armanio123 pushed a commit to armanio123/typescript-eslint that referenced this pull request Jan 24, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[no-object-literal-type-assertion] False positive when asserting function argument
5 participants