Skip to content

Rule proposal: Prevent Object.values(...) usage with Map #6807

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

Open
6 tasks done
MadLittleMods opened this issue Mar 31, 2023 · 5 comments
Open
6 tasks done

Rule proposal: Prevent Object.values(...) usage with Map #6807

MadLittleMods opened this issue Mar 31, 2023 · 5 comments
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@MadLittleMods
Copy link

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

  • My proposal specifically checks TypeScript syntax, or it proposes a check that requires type information to be accurate.
  • My proposal is not a "formatting rule"; meaning it does not just enforce how code is formatted (whitespace, brace placement, etc).
  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Description

Using Object.values(...), Object.keys(...), Object.entries(...) with a Map object is a foot-gun because it will return empty [] regardless of the map contents and is most likely a mistake. These sorts of mistakes are easy to get into during refactors. This issue is spawning from a real-life refactor mistake -> element-hq/element-web#24995 (comment)

Related StackOverflow question: https://stackoverflow.com/questions/72315392/how-to-prevent-the-mistake-of-calling-object-values-on-a-map

As @bergus mentions from the SO question, it's possible to extend Map to make a subclass that returns something with the object utilities but this seems a lot more niche compared to the big problem of running them on base Map objects. AFAICT, it's not possible to use TypeScript on its own to catch this sort of thing because practically everything inherits from Object and function arguments in TypeScript are contravariant (accepts supertypes but doesn't accept subtypes).

Fail Cases

const map = new Map([[ 1, 'one' ],[ 2, 'two' ]]);

// Rule would error for each of these usages, as using these object utilities
// on a Map will just return empty results and is probably a mistake
Object.keys(map); // -> []
Object.values(map); // -> []
Object.entries(map); // -> []

Pass Cases

const myObject = { 1: 'one', 2: 'two' };
Object.keys(myObject); // -> ['1', '2']
Object.values(myObject); // -> ['one', 'two']
Object.entries(myObject); // -> [[ 1, 'one' ],[ 2, 'two' ]]

const map = new Map([[ 1, 'one' ],[ 2, 'two' ]]);
map.keys(); // -> ['1', '2']
map.values(); // -> ['one', 'two']
map.entries(); // -> [[ 1, 'one' ],[ 2, 'two' ]]

Additional Info

No response

@MadLittleMods MadLittleMods added enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Mar 31, 2023
@Josh-Cena
Copy link
Member

This presents an interesting intersection with no-misused-in proposed in #5677. They both deal with attempting to use maps/sets as if they have object properties. I wonder how we could make the one proposed here more generalizable.

@bradzacher
Copy link
Member

Heh this request reminded me of that exact same issue! Yeah there's something interesting here because they're all related but also distinct problem sets.

Maybe a big rule like no-misused-object-likes would be a good thing?

@SimonSchick
Copy link

I'm somewhat interested in picking this up again since it yet-again managed to get into production 🙄

Any tips on how to identity the Map/Set type more systematically? If not maybe we can move ahead with what @gibson suggested and iterate on the solution.

Initially just capturing Array, Set, Headers, Map (and their respectively readonly variants) may suffice.

@JoshuaKGoldberg
Copy link
Member

#10104 has some good starting code for this issue. #10551 might also be useful as a reference for another rule that deals with built-in classes.

@jdufresne
Copy link

jdufresne commented Mar 12, 2025

I'd like to throw out another potential case of a misused Object.values() when it is called on an array: Object.values(array). This is a construct that I have found in my own projects. Once found, I simplify it to be just the array (or a copy of the array).

// fail
Object.values(['a', 'b', 'c'])  // simplify to: ['a', 'b', 'c']

const array = ['a', 'b', 'c']
Object.values(array)  // simplify to: array


// pass
type MyObject = { a: string }

let arrayOrObject: array | MyObject
arrayOrObject = ['a', 'b', 'c']
Object.values(arrayOrObject)  // could be an object, can't simplify

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
6 participants