eslint / eslint

Find and fix problems in your JavaScript code.
https://eslint.org
MIT License
24.44k stars 4.41k forks source link

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

Closed pubmikeb closed 2 years ago

pubmikeb commented 2 years ago

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.

nzakas commented 2 years ago

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

bmish commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

fisker commented 2 years ago

Will be happy to see the rule in core rules.

aladdin-add commented 2 years ago

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

pubmikeb commented 2 years ago

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 commented 2 years ago

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

mdjermanovic commented 2 years ago

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

ZhangChengLin commented 2 years ago

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 commented 2 years ago

I’m also supportive. Marking as accepted!

thisiskartikgupta commented 2 years ago

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

snitin315 commented 2 years ago

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

jamiebuilds commented 2 years ago

FYI: Object.hasOwn() just reached Stage 4

eyeamkd commented 2 years ago

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

nzakas commented 2 years ago

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 commented 2 years ago

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

thisiskartikgupta commented 2 years ago

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

nzakas commented 2 years ago

@eyeamkd it’s all yours.

eyeamkd commented 2 years ago

@eyeamkd it’s all yours.

Thank you, @nzakas

eyeamkd commented 2 years ago

Screenshot 2021-09-14 at 7 17 30 AM

doing npm install gives me this, is it okay to do npm i -f and continue? prior apologies if this is a trivial question

btmills commented 2 years ago

@eyeamkd yes, npm install --force will do the trick until we release v8.0.0 final

eyeamkd commented 2 years ago

Worked @btmills

bmish commented 2 years ago

Just a reminder that there's an existing rule unicorn/prefer-object-has-own that we should be able to copy (and it has been battle-tested for several months).

eyeamkd commented 2 years ago

Alright, so all I gotta do is just take the rule and apply conditions on to it? @bmish

bmish commented 2 years ago

Not sure what you mean by "apply conditions on to it". But I would first try to copy it and get it running inside the ESLint codebase.

eyeamkd commented 2 years ago

Okay!

nzakas commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

bmish commented 2 years ago

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 commented 2 years ago

@bmish 👍

AtulKarn commented 2 years ago

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 commented 2 years ago

Yeah, will place a PR soon @AtulKarn

AtulKarn commented 2 years ago

Oh that's great

Gautam-Arora24 commented 2 years ago

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

pubmikeb commented 2 years ago

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

Yes, sure! Go for it.

nzakas commented 2 years ago

@Gautam-Arora24 go for it!

Gautam-Arora24 commented 2 years ago

Thanks!

eyeamkd commented 2 years ago

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 commented 2 years ago

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

mdjermanovic commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Seems reasonable to me. 👍

bharathks005 commented 2 years ago

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

snitin315 commented 2 years ago

It is currently being implemented in https://github.com/eslint/eslint/pull/15346.