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

A rule suggesting to replace Object.prototype.hasOwnProperty.call with Object.hasOwn #14939

Closed
pubmikeb opened this issue Aug 16, 2021 · 46 comments · Fixed by #15346
Closed

A rule suggesting to replace Object.prototype.hasOwnProperty.call with Object.hasOwn #14939

pubmikeb opened this issue Aug 16, 2021 · 46 comments · Fixed by #15346
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion feature This change adds a new feature to ESLint good first issue Good for people who haven't worked on ESLint before rule Relates to ESLint's core rules
Projects

Comments

@pubmikeb
Copy link

pubmikeb commented Aug 16, 2021

Please describe what the rule should do:
Starting V8 v.9.3, Object.prototype.hasOwnProperty.call can be replaced with an alias/syntax sugar Object.hasOwn, which is much more read-friendly. Further information: https://v8.dev/features/object-has-own

What new ECMAScript feature does this rule relate to?
Promoting using of an alias/syntax sugar Object.hasOwn instead of Object.prototype.hasOwnProperty.call.

What category of rule is this? (place an "X" next to just one item)

[ ] Warns about a potential error (problem)
[X] Suggests an alternate way of doing something (suggestion)
[ ] Other (please specify:)

Provide 2-3 code examples that this rule will warn about:
The original code:

if (Object.prototype.hasOwnProperty.call(object, 'foo')) {
  // `object` has property `foo`.
}

Should be replaced with:

if (Object.hasOwn(object, 'foo')) {
  // `object` has property `foo`.
}

Why should this rule be included in ESLint (instead of a plugin)?
Applying this rule you can make your application's code cleaner.

Are you willing to submit a pull request to implement this rule?
No.

@pubmikeb pubmikeb added feature This change adds a new feature to ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon labels Aug 16, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Aug 16, 2021
@nzakas nzakas removed the triage An ESLint team member will look at this issue soon label Aug 17, 2021
@nzakas
Copy link
Member

nzakas commented Aug 17, 2021

I will champion this rule, as I think it will help people better understand this new method.

@nzakas nzakas self-assigned this Aug 17, 2021
@nzakas nzakas moved this from Needs Triage to Feedback Needed in Triage Aug 17, 2021
@bmish
Copy link
Sponsor Member

bmish commented Aug 17, 2021

FYI this rule was implemented in another project as unicorn/prefer-object-has-own but I suspect @fisker would be happy to see it moved to ESLint core where it can benefit more people.

@nzakas
Copy link
Member

nzakas commented Aug 17, 2021

@bmish oh cool! We do typically introduce new rules to help people transition from old ways of doing things to new ways of doing things, so we might still end up with it as a core rule. Let’s get a few more thoughts on this. (Especially since there isn’t a great way to discover such rules and plug-ins.)

@bmish
Copy link
Sponsor Member

bmish commented Aug 17, 2021

Yep, I'm also in favor of new core rules for that reason (like this one).

@fisker
Copy link
Contributor

fisker commented Aug 17, 2021

Will be happy to see the rule in core rules.

@aladdin-add
Copy link
Member

proposal-accessible-object-hasownproperty is at stage-3. we can reconsider it when it reaches stage-4.

@pubmikeb
Copy link
Author

pubmikeb commented Aug 17, 2021

proposal-accessible-object-hasownproperty is at stage-3. we can reconsider it when it reaches stage-4.

Since this feature is already implemented in V8 and will be in GA in Chromium-based browsers/Firefox/Safari/Node.js by the end of August, the reaching stage-4 is just a formality and by the release of ESLint 8.0 Object.hasOwn() will be in GA across all major platforms.

@nzakas
Copy link
Member

nzakas commented Aug 17, 2021

@aladdin-add we wait for stage 4 for new syntax, but new APIs are okay to address in stage 3.

@mdjermanovic
Copy link
Member

I'm in favor of this rule, and I think it should also report {}.hasOwnProperty.call(object, 'foo').

@ZhangChengLin
Copy link

ZhangChengLin commented Aug 19, 2021

I'm in favor of this rule, and I think it should also report {}.hasOwnProperty.call(object, 'foo').

{}.hasOwnProperty.call(object, 'foo')

Object.prototype.hasOwnProperty.call(object,'foo')

function fun(){
    return arguments.length
}

How to write arguments so as not to be prompted no-undef

@btmills
Copy link
Member

btmills commented Aug 21, 2021

I’m also supportive. Marking as accepted!

@btmills btmills added the accepted There is consensus among the team that this change meets the criteria for inclusion label Aug 21, 2021
@btmills btmills moved this from Feedback Needed to Ready to Implement in Triage Aug 21, 2021
@nzakas nzakas added the good first issue Good for people who haven't worked on ESLint before label Aug 24, 2021
@thisiskartikgupta
Copy link

Hello everyone, I am a beginner to open-source. I have some experience in working with Javascript. Can I work on this issue?

@snitin315
Copy link
Contributor

@thisiskartikgupta sure, let us know if you need any help.

@jamiebuilds
Copy link

FYI: Object.hasOwn() just reached Stage 4

@eyeamkd
Copy link

eyeamkd commented Sep 13, 2021

Is the PR as simple as just replacing the code? Or is there any other problem to consider?

@nzakas
Copy link
Member

nzakas commented Sep 14, 2021

The rule first needs to identify the patterns mentioned in the issue. The second step is to replace. It is fairly straightforward.

@thisiskartikgupta are you still working on this?

@eyeamkd
Copy link

eyeamkd commented Sep 14, 2021

Understood @nzakas. If @thisiskartikgupta isn't currently working on this, can you please assign this to me?

@thisiskartikgupta
Copy link

I'm not working on it. You're good to go @eyeamkd !!

@nzakas
Copy link
Member

nzakas commented Sep 14, 2021

@eyeamkd it’s all yours.

@eyeamkd
Copy link

eyeamkd commented Sep 14, 2021

@eyeamkd it’s all yours.

Thank you, @nzakas

@eyeamkd
Copy link

eyeamkd commented Sep 15, 2021

Okay!

@nzakas
Copy link
Member

nzakas commented Sep 16, 2021

Hold on! We aren’t just going to copy someone else’s work and paste it into ESLint. We should create our own to avoid licensing issues.

This is a fairly straightforward rule that shouldn’t require anything complex enough that it requires copying something directly.

@nzakas
Copy link
Member

nzakas commented Sep 16, 2021

@eyeamkd if you need a starting point, you can copy this rule:

https://eslint.org/docs/rules/prefer-object-spread

The logic for finding Object.assign is exactly the same as for Object.hasOwn.

@eyeamkd
Copy link

eyeamkd commented Sep 16, 2021

Using the same @nzakas, and yeah won't be copy-pasting anyone else's work

@bmish
Copy link
Sponsor Member

bmish commented Sep 16, 2021

Let me try again with better advice :) Write our own rule and then check any existing third-party rules to make sure we don't miss important test cases.

@nzakas
Copy link
Member

nzakas commented Sep 16, 2021

@bmish 👍

@AtulKarn
Copy link

AtulKarn commented Oct 1, 2021

Hi! is @eyeamkd still working on this? If not then can I take this up? I am new to open source and this seemed a good issue to work on

@eyeamkd
Copy link

eyeamkd commented Oct 1, 2021

Yeah, will place a PR soon @AtulKarn

@AtulKarn
Copy link

AtulKarn commented Oct 1, 2021

Oh that's great

@Gautam-Arora24
Copy link
Contributor

If no one is working on this issue, can I start with it? 🙂

@pubmikeb
Copy link
Author

If no one is working on this issue, can I start with it? 🙂

Yes, sure! Go for it.

@nzakas
Copy link
Member

nzakas commented Oct 22, 2021

@Gautam-Arora24 go for it!

@Gautam-Arora24
Copy link
Contributor

Thanks!

@eyeamkd
Copy link

eyeamkd commented Oct 22, 2021

Hey @nzakas, I have been getting an error while running the repo.
I've asked it on the ESLint Discord channel too, can you help me out here?
https://discord.com/channels/688543509199716507/717416886685401108/900029930526621767

@nzakas
Copy link
Member

nzakas commented Oct 23, 2021

It looks like you don’t have an updated repo. Make sure you sync with upstream, then delete your node_modules directory and reinstall.

Gautam-Arora24 added a commit to Gautam-Arora24/eslint that referenced this issue Oct 24, 2021
Gautam-Arora24 added a commit to Gautam-Arora24/eslint that referenced this issue Oct 24, 2021
@mdjermanovic
Copy link
Member

We've agreed on reporting these patterns:

Object.prototype.hasOwnProperty.call(object, 'foo')

{}.hasOwnProperty.call(object, 'foo')

Thoughts about reporting this pattern, too?

Object.hasOwnProperty.call(object, 'foo')

@pubmikeb
Copy link
Author

Thoughts about reporting this pattern, too?

Object.hasOwnProperty.call(object, 'foo')

Yes, i would add this pattern as well.

Generally, I would propose replacing *.hasOwnProperty.* with *.hasOwn.*.

@nzakas
Copy link
Member

nzakas commented Oct 28, 2021

Seems reasonable to me. 👍

Gautam-Arora24 added a commit to Gautam-Arora24/eslint that referenced this issue Oct 28, 2021
@bharathks005
Copy link

Is the issue is not fixed, can I start to fix this issue? can I try?

@snitin315
Copy link
Contributor

It is currently being implemented in #15346.

@mdjermanovic mdjermanovic moved this from Ready to Implement to Pull Request Opened in Triage Dec 2, 2021
Triage automation moved this from Pull Request Opened to Complete Dec 11, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 10, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion feature This change adds a new feature to ESLint good first issue Good for people who haven't worked on ESLint before rule Relates to ESLint's core rules
Projects
Archived in project
Triage
Complete