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

refactor: simplify props function #19

Closed
wants to merge 1 commit into from

Conversation

johnhooks
Copy link
Contributor

@johnhooks johnhooks commented Mar 14, 2023

@aduth is getPath necessary?

It looks like it's only ever used to look up a single key of "attributes". In PR #18 It's difficult to type as path should be some sort of nested keyof type, but there is no way to do that with a dot separated list. It is probably possible with an array of keys. But again, if getPath can be replace with the following, it makes typing props much simpler:

export function prop(selector, name) {
	if (1 === arguments.length) {
		name = selector;
		selector = undefined;
	}

	return function (node) {
		let match = node;
		if (selector) {
			match = node.querySelector(selector);
		}

		if (match && name in match) {
			return match[name];
		}
	};
}

Does this fundamentally change how the library is use in real life? Does any consumer of the package use a dot separated list to access nested properties?

The I don't see any where in documentation this feature is mentioned.

@aduth
Copy link
Owner

aduth commented Mar 15, 2023

The fact that all tests pass isn't a good sign for this being important 😅 Though I think that's more that the test coverage is lacking for the intended use-case. If I recall, the idea here is to allow for nested property access for a matcher, for example grabbing data- attribute values out of an element's dataset property.

Here's an example in the playground environment:

image

Unsure how valuable this would be since at least in this example, the equivalent value could be derived using the data-foo HTML attribute. That said, I'd still worry this would be a breaking change, and might have some undesirable downstream impact on consuming projects like Gutenberg.

@aduth
Copy link
Owner

aduth commented Mar 15, 2023

This utility behaves very similar to Lodash's get. I wonder if typings associated with that function could serve as inspiration for how to address this?

@aduth
Copy link
Owner

aduth commented Mar 15, 2023

Alternatively, as far as it impacts typings, if it makes it easier for you to define types conforming to the behavior you're proposing even if it still technically supports dot-separated paths under the hood, I think that'd be fine too?

@johnhooks
Copy link
Contributor Author

johnhooks commented Mar 15, 2023

This is the typing for lodash get. It's a simple utility in implementation, but very difficult to type. It looks like it can split the dot separated list, but its a very complicated type.

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/11d348eeeb9f2accce5004821885397f90ab5da6/types/lodash/common/object.d.ts#L1059-L1146

The link below is how they break apart the dot separated path.

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/11d348eeeb9f2accce5004821885397f90ab5da6/types/lodash/common/object.d.ts#L1023-L1057

I didn't know something like that could be done.

@johnhooks
Copy link
Contributor Author

johnhooks commented Mar 15, 2023

I don't like the idea of typing it in a way that doesn't actually fit it's actual usage. I'll play with some other ideas. I only proposed removing it because I didn't see it used or documented with that functionality.

@johnhooks johnhooks closed this Mar 15, 2023
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.

2 participants