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

Add mapObject.mapObjectSkip for removing object keys #38

Merged
merged 11 commits into from Sep 19, 2021
Merged

Add mapObject.mapObjectSkip for removing object keys #38

merged 11 commits into from Sep 19, 2021

Conversation

gr2m
Copy link
Contributor

@gr2m gr2m commented Sep 4, 2021

closes #36

I need help with the types please. The setup is different from https://github.com/sindresorhus/p-map/blob/main/index.d.ts, and I wasn't able to figure out how to define the types for the mapObject.mapObjSkip symbol

@gr2m gr2m mentioned this pull request Sep 4, 2021
index.js Outdated Show resolved Hide resolved
index.d.ts Outdated
@@ -45,6 +45,8 @@ declare namespace mapObject {
*/
shouldRecurse?: boolean;
}

export type mapObjSkip = symbol
Copy link
Owner

Choose a reason for hiding this comment

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

This is not correct. I would recommend reading up on unique symbol in TypeScript.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw it implemented at https://github.com/sindresorhus/p-map/blob/main/index.d.ts#L93 and I found the docs for it at https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-7.html#unique-symbol.

But this is not an ES Module yet, so we can't just do this, right?

export const mapObjectSkip: unique symbol;

I think we need to define the mapObjectSkip on the mapObject namespace, and I tried several approaches, but couldn't get it to work.

How would you do it?

Copy link
Owner

Choose a reason for hiding this comment

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

Just use declare const at the top-level to declare the unique symbol and then add it to the namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done via 1efe1a5

@sindresorhus
Copy link
Owner

You need to add proper readme docs for the symbol, including description on how it should be used and what it does.

@gr2m
Copy link
Contributor Author

gr2m commented Sep 4, 2021

You need to add proper readme docs for the symbol, including description on how it should be used and what it does.

done via a5342ee

@brandon93s brandon93s mentioned this pull request Sep 4, 2021
1 task
@sindresorhus
Copy link
Owner

The pull request also needs a proper title.

@gr2m gr2m changed the title mapObject.mapObjSkip Add mapObject.mapObjectSkip for removing object keys Sep 5, 2021
@gr2m
Copy link
Contributor Author

gr2m commented Sep 5, 2021

The pull request also needs a proper title.

I changed it but feel free to change it to your own liking, that's what I do for outside contributions in my own projects.

index.d.ts Outdated Show resolved Hide resolved
@@ -65,6 +67,9 @@ const newObject = mapObject({FOO: true, bAr: {bAz: true}}, (key, value) => [key.

const newObject = mapObject({FOO: true, bAr: {bAz: true}}, (key, value) => [key.toLowerCase(), value], {deep: true});
//=> {foo: true, bar: {baz: true}}

const newObject = mapObject({one: 1, two: 2}, (key, value) => value === 1 ? [key, value] : mapObject.mapObjectSkip);
//=> {one: 1}
Copy link
Owner

Choose a reason for hiding this comment

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

The example could be in the symbol doc comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is

	/**
	Return this value from a `mapper` function to remove a key from an object.

	```js
	const mapObject = require('map-obj');

	const object = {one: 1, two: 2}
	const mapper = (key, value) => value === 1 ? [key, value] : mapObject.mapObjectSkip
	const result = mapObject(object, mapper);

	console.log(result);
	//=> {one: 1}
	```
	*/
	const mapObjectSkip: typeof UniqueSymbol

readme.md Outdated Show resolved Hide resolved
index.d.ts Outdated
}

// unique symbol cannot be declared in a namespace directly, so we declare it top-level
// See: https://github.com/sindresorhus/map-obj/pull/38#discussion_r702396878
declare const UniqueSymbol: unique symbol;
Copy link
Owner

Choose a reason for hiding this comment

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

It should still have a descriptive name and be camel-cased.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

camel cased via 358fafd.

It's only used internally, and for that I think it's perfectly descriptive? Can you rename it yourself to whatever you prefer at this point?

Copy link
Owner

Choose a reason for hiding this comment

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

Even it's internally, it's good to have descriptive names. You don't call all your variables const variable1 = …; const variable2 .... But it's not a problem. I can clean it up later.

@sindresorhus sindresorhus merged commit 9ee1876 into sindresorhus:main Sep 19, 2021
@sindresorhus
Copy link
Owner

Thanks :)

@gr2m gr2m deleted the 36/remove-key branch September 19, 2021 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to remove a key
2 participants